Skip to content

feat(apikey): scoped, rotatable API keys (Tier-A #3)#16

Merged
dedeez14 merged 4 commits intomainfrom
devin/1777089907-apikey-auth
Apr 25, 2026
Merged

feat(apikey): scoped, rotatable API keys (Tier-A #3)#16
dedeez14 merged 4 commits intomainfrom
devin/1777089907-apikey-auth

Conversation

@devin-ai-integration
Copy link
Copy Markdown
Contributor

@devin-ai-integration devin-ai-integration Bot commented Apr 25, 2026

Summary

Adds service-to-service API-key authentication that composes with the existing JWT + RBAC stack. The bearer can be either a JWT (unchanged behaviour) or a goforge-format API key gf_<env>_<id>_<secret>; routes downstream of the auth group are oblivious to which one was presented.

Why

Tier-A #3 of the Tier A + Tier B framework expansion. Almost every SaaS / B2B integration needs scoped, rotatable, prefixed-id API keys (Stripe-style). Hand-rolling them per project wastes a day every time and is easy to get wrong (timing leaks, missing rotation, denormalised vs role-derived scopes).

How

Token format

gf_<env>_<id>_<secret>
 |   |     |    |
 |   |     |    +-- 64 hex chars (256-bit secret entropy)
 |   |     +------- 12 hex chars (48-bit unique id, indexed)
 |   +------------- env tag ("live" / "staging" / "dev" — visible)
 +----------------- framework tag ("gf" — never changes)

The visible env tag exists so a leaked key is immediately identifiable (it's why GitHub keys say ghp_… and Stripe says sk_live_…).

Storage / hashing

Only sha256(plaintext) is stored. Plaintext is returned by POST /api/v1/api-keys exactly once and never persisted.

SHA-256 is the right choice here — not Argon2id (used elsewhere for password hashing). The secret already carries 256 bits of cryptographic entropy; the cost of a memory-hard hash buys nothing without a low-entropy input. Constant-time hex compare prevents timing leaks against the prefix lookup.

Layers

  • pkg/apikey/Generate / Parse / VerifyHash / LooksLikeAPIKey (zero infra deps)
  • internal/domain/apikey/Key entity, Repo interface, ErrNotFound
  • internal/adapter/repository/postgres/ — pgxpool implementation
  • internal/usecase/apikey.goCreate / List / Revoke / Authenticate
  • internal/adapter/http/middleware/apikey.goAPIKeyOrJWTAuth
  • internal/adapter/http/handler/apikey.go/api/v1/api-keys CRUD
  • migrations/0008_api_keys.{up,down}.sql

Routes (behind the existing authenticated group)

  • GET /api/v1/api-keys — list caller's own keys
  • POST /api/v1/api-keys — mint key, plaintext returned exactly once
  • DELETE /api/v1/api-keys/:id — revoke

RBAC interaction (the interesting bit)

RequirePermission("x.y", ...) short-circuits when the request arrived with a goforge API key: it consults the key's scopes, not the owning user's role surface. So a user with role admin can mint a key with scope ["reports.read"] for a CI job and that key cannot perform admin actions. The wildcard "*" is honoured for break-glass keys; the deployment is responsible for gating who may issue them.

Test plan

  • make lint clean (golangci-lint run ./...)
  • make test clean (go test -race ./...)
  • Migrations applied + rolled back successfully
  • Tests cover:
    • pkg/apikey: format, round-trip, tampered-secret rejection
    • internal/usecase: happy path, tampered, revoked, expired, malformed-bearer-is-401, NotFound mapping, scope sanitisation
    • internal/adapter/http/middleware: scope-grants-without-RBAC-lookup, narrow-scope-denies-even-if-role-would-grant, wildcard, JWT fallback path leaves API-key auth untouched, 401 on bad bearer

Risk

  • Low — additive feature. The new middleware short-circuit only activates when the composition root passes APIKeyAuth in AccessControl; existing apps wired without it keep behaving exactly as before.
  • The new middleware sits before Auth in the chain. Re-using c.Locals(CtxKeyUserID) keeps every downstream handler unaware of the auth mechanism.
  • Rollback: revert PR + run migrations/0008_api_keys.down.sql. No public API changes outside the new package.

Checklist

  • Doc comments updated for any public API change (full doc comments on every exported symbol; docs/api-keys.md covers token format, RBAC interaction, schema)
  • OpenAPI 3.1 doc auto-registers the three new operations under the api-keys tag
  • No secrets / debug logs / fmt.Println left behind

Link to Devin session: https://app.devin.ai/sessions/8fdfc20358514c97a766adca630a2527
Requested by: @dedeez14


Open in Devin Review

Adds service-to-service API-key authentication that composes with
the existing JWT + RBAC stack. The bearer can be either a JWT
(unchanged behaviour) or a goforge-format API key
(`gf_<env>_<id>_<secret>`); routes downstream of the auth group
are oblivious to which one was presented.

Token format
  gf_<env>_<id>_<secret>
   |   |     |    |
   |   |     |    +-- 64 hex chars (256-bit secret entropy)
   |   |     +------- 12 hex chars (48-bit unique id, indexed)
   |   +------------- env tag ("live" / "staging" / "dev" - visible)
   +----------------- framework tag ("gf" - never changes)

Storage holds only sha256(plaintext); the plaintext is returned by
POST /api/v1/api-keys exactly once and never persisted. SHA-256 is
appropriate (not Argon2id) because the secret already carries 256
bits of entropy; the cost of memory-hard hashing buys nothing
without a low-entropy input. Constant-time hex compare prevents
timing leaks against the prefix lookup.

Layers added (clean-arch pattern):
  pkg/apikey/                    Generate / Parse / VerifyHash / LooksLikeAPIKey
  internal/domain/apikey/        Key entity + Repo interface + ErrNotFound
  internal/adapter/repository/   pgxpool implementation
  internal/usecase/apikey.go     Create / List / Revoke / Authenticate
  internal/adapter/http/dto/     CreateAPIKey{Request,Response}
  internal/adapter/http/handler/ /api/v1/api-keys CRUD
  internal/adapter/http/middleware/apikey.go   APIKeyOrJWTAuth
  migrations/0008_api_keys.{up,down}.sql

Routes (all behind the existing authenticated group, available
whenever the composition root passes APIKeyAuth in AccessControl):
  GET    /api/v1/api-keys          list caller's own keys
  POST   /api/v1/api-keys          mint new key (plaintext returned 1x)
  DELETE /api/v1/api-keys/:id      revoke

RBAC interaction
  RequirePermission("x.y", ...) short-circuits when the request
  arrived with a goforge API key: it consults the *key's scopes*,
  not the owning user's role surface. This is by design - a service
  key with scope=["reports.read"] cannot piggyback on the owning
  user's admin role. The wildcard "*" is honoured for break-glass
  keys; the deployment is responsible for gating who may issue them.

Tests
  - pkg/apikey: format, round-trip, tampered-secret rejection
  - internal/usecase: happy path, tampered, revoked, expired,
    malformed-bearer-is-401, NotFound mapping, scope sanitisation
  - internal/adapter/http/middleware: scope-grants-without-RBAC,
    narrow-scope-denies-even-if-role-would-grant, wildcard,
    JWT fallback path leaves API-key auth untouched, 401 on bad bearer

Composition root (internal/app/app.go) wires APIKeyRepository ->
APIKeyUseCase -> Handler -> Router, deriving the env tag from the
configured app environment ("live" in production).

OpenAPI 3.1 doc auto-registers the three new operations under the
"api-keys" tag.

go test -race ./... clean.
golangci-lint run ./... clean.

Co-Authored-By: dede febriansyah <febriansyahd65@gmail.com>
@devin-ai-integration
Copy link
Copy Markdown
Contributor Author

🤖 Devin AI Engineer

I'll be helping with this pull request! Here's what you should know:

✅ I will automatically:

  • Address comments on this PR. Add '(aside)' to your comment to have me ignore it.
  • Look at CI failures and help fix them

Note: I can only respond to comments from users who have write access to this repository.

⚙️ Control Options:

  • Disable automatic comment and CI monitoring

devin-ai-integration[bot]

This comment was marked as resolved.

)

The original use-case Revoke took only the key id and the actor; it
forwarded directly to the repo, whose SQL only matched on id. As a
result any authenticated user could revoke any other user's key by
knowing (or enumerating) its UUID - a denial-of-service vector and
direct violation of the documented "operate against the caller's
own keys" contract.

Took the SQL-level option from the review prompt (preferred over a
two-statement check + update because it eliminates the read/write
race between ownership check and revoke):

  qAPIKeyRevoke
      WHERE id = $1
        AND user_id = $2
        AND revoked_at IS NULL
        AND deleted_at IS NULL

  Repo:    Revoke(ctx, id, ownerID, by, at)
  UseCase: Revoke(ctx, id, ownerID, by)
  Handler: passes the authenticated uid as both ownerID and audit
           actor ("I revoke my own key")

Mismatched ownership collapses into ErrNotFound rather than
Forbidden so the endpoint cannot be used as an oracle for key
existence; the response body is identical to a truly missing key.

System keys (user_id IS NULL) are deliberately *not* revocable
through this endpoint - those belong to an admin tool that should
gate revoke behind apikeys.manage and pass its own filter.
Documented in docs/api-keys.md.

Tests:
  - TestRevoke_RejectsForeignOwner    -- regression for the IDOR
  - TestRevoke_AcceptsOwner           -- happy path stays green
  - existing TestAuthenticate_RejectsRevokedKey + TestRevoke_NotFound
    updated to pass an explicit owner

go test -race ./... clean, golangci-lint clean.

Co-Authored-By: dede febriansyah <febriansyahd65@gmail.com>
devin-ai-integration[bot]

This comment was marked as resolved.

… PR #16)

Trace confirmed: the use-case built the Key in memory without
stamping CreatedAt, the repo INSERT used NOW() in SQL with no
RETURNING, and the in-memory struct then flowed straight into the
response DTO - so every successful POST /api/v1/api-keys serialised
"created_at": "0001-01-01T00:00:00Z".

Took option 1 from the review prompt (in-memory clock-stamp +
parameterise the SQL) over RETURNING because:
  - the use-case already exposes an injectable clock for tests
  - we avoid the round-trip and the additional Scan plumbing
  - it mirrors UserRepository.Create / RoleRepository.Create

Changes:
  - usecase/apikey.go: stamp CreatedAt = UpdatedAt = u.clock().UTC()
    before handing the struct to the repo
  - postgres/apikey.go: INSERT now binds $9 = created_at/updated_at
    (was NOW()), $10 = created_by/updated_by; defensive zero-time
    guard for callers that bypass the use-case (seed scripts)
  - new test TestCreate_StampsTimestamps asserts the response DTO
    carries the clock value, not the zero time

Pre-existing equivalents in RoleRepository / PermissionRepository
are mentioned in the review prompt but out of scope for this PR;
filed mentally as a follow-up.

go test -race ./... clean, golangci-lint clean.

Co-Authored-By: dede febriansyah <febriansyahd65@gmail.com>
devin-ai-integration[bot]

This comment was marked as resolved.

Two Devin Review findings on PR #16, both real:

1) [Doc] Migration comment claimed Argon2id, implementation uses SHA-256.
   docs/api-keys.md and the domain doc-comment are correct; only the
   migration was wrong. Updated to match.

2) [Convention] apikey.ErrNotFound was a bare errors.New(...) sentinel,
   diverging from user.ErrNotFound / menu.ErrNotFound which are
   pre-built *errs.Error values. AGENTS.md mandates *pkg/errs.Error
   for client-visible errors; the bare sentinel relied on a
   MapNotFound() helper at the use-case boundary, and any future
   path that forgot to call it would have surfaced a generic 500.

   Switched ErrNotFound to errs.NotFound("apikey.not_found", "API
   key not found"), removed MapNotFound(), and simplified
   APIKeyUseCase.Revoke to forward the repo's error directly. The
   Authenticate path was already opaque-401-by-design and is
   unaffected.

   Pointer equality of the singleton means errors.Is(err, ErrNotFound)
   keeps working for any future caller that wants to distinguish.

go test -race ./... clean, golangci-lint clean.

Co-Authored-By: dede febriansyah <febriansyahd65@gmail.com>
Copy link
Copy Markdown
Contributor Author

@devin-ai-integration devin-ai-integration Bot left a comment

Choose a reason for hiding this comment

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

Devin Review found 1 new potential issue.

View 10 additional findings in Devin Review.

Open in Devin Review

Comment on lines +64 to +68
if h.APIKeys != nil {
authed.Get("/api-keys", h.APIKeys.List)
authed.Post("/api-keys", h.APIKeys.Create)
authed.Delete("/api-keys/:id", h.APIKeys.Revoke)
}
Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

🔴 API-key-authenticated requests can mint new keys with arbitrary scopes, enabling privilege escalation

The /api-keys self-service routes (internal/infrastructure/server/router.go:65-68) are registered under the authed group with no RequirePermission middleware. This means an API key with narrow scopes (e.g., ["reports.read"]) can call POST /api/v1/api-keys and create a new key with ["*"] scopes, completely bypassing the scope restriction. The Create handler (internal/adapter/http/handler/apikey.go:64-69) passes req.Scopes directly from the request body to the use case with no check that the caller's own scopes are a superset of the requested scopes.

This defeats the core security invariant stated in the PR's own docs and middleware comments: "a service key with a narrow scope can never piggyback on the owning user's broader role assignments." While RequirePermission correctly short-circuits on API-key scopes for gated routes, the ungated key-management endpoints let a narrow key escalate to wildcard access in one API call.

Prompt for agents
The API key management endpoints (GET/POST/DELETE /api-keys) are registered under the authed group with no scope or permission check. This allows any API-key-authenticated request to create new keys with arbitrary scopes, escalating privileges.

Relevant files:
- internal/infrastructure/server/router.go:64-68 (route registration)
- internal/adapter/http/handler/apikey.go:51-78 (Create handler passes req.Scopes unchecked)
- internal/adapter/http/middleware/apikey.go:41-65 (sets scopes in context)
- internal/adapter/http/middleware/permission.go:71-78 (scope check only runs when RequirePermission is used)

Possible approaches:
1. Gate the api-keys routes behind a RequirePermission("apikeys.manage", ...) middleware so that API keys need an explicit scope to manage keys.
2. In the Create handler, when the request is API-key-authenticated (APIKeyScopesFromCtx returns non-nil), reject the request or validate that the requested scopes are a subset of the caller's scopes.
3. Reject API-key-authenticated requests from the key management endpoints entirely (only allow JWT).
Open in Devin Review

Was this helpful? React with 👍 or 👎 to provide feedback.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Real and critical — confirmed and fixed in f0e4d1f.

Took approach 3 (forbid API-key auth on the whole /api-keys subtree) rather than a subset-scope check because the strict rule is the only one that's safe by default:

  • Subset-check still lets a leaked key call DELETE /api-keys/:id and revoke the owner's other keys (denial of service).
  • Subset-check still lets a leaked key rotate itself, escaping the owner's revoke.
  • Strict rule is structural and auditable — "credential management is only from JWT sessions", no carve-outs.

Implementation:

// middleware/apikey.go - new guard
func RequireUserSession() fiber.Handler {
    return func(c *fiber.Ctx) error {
        if v := c.Locals(CtxKeyAPIKeyScopes); v != nil {
            return httpx.RespondError(c,
                errs.Forbidden(
                    "apikey.user_session_required",
                    "this endpoint cannot be called with an API key; sign in to manage credentials",
                ))
        }
        return c.Next()
    }
}

// server/router.go
if h.APIKeys != nil {
    keys := authed.Group("/api-keys", middleware.RequireUserSession())
    keys.Get("",     h.APIKeys.List)
    keys.Post("",    h.APIKeys.Create)
    keys.Delete("/:id", h.APIKeys.Revoke)
}

Regression test TestRequireUserSession_RejectsAPIKeyAuth covers both legs — API-key bearer gets 403, JWT bearer passes through.

Future admin tooling that legitimately needs programmatic key management can use a JWT-exchange flow instead of a scope — that keeps the attack surface of a leaked API key from ever including self-replication.

@dedeez14 dedeez14 merged commit 45ce58c into main Apr 25, 2026
3 checks passed
devin-ai-integration Bot added a commit that referenced this pull request Apr 25, 2026
…#16)

Critical finding confirmed. The /api-keys self-service routes were
registered under the plain authed group with no structural guard,
so a narrow-scoped API key could call POST /api/v1/api-keys with
scopes=["*"] and mint a wildcard successor for itself - one-hop
privilege escalation that defeats the PR's own invariant that a
service key's scope is an upper bound on what the caller can do.

Took the strict option (approach 3 from the review prompt) because
it's the only one that's safe by default:

  - RequireUserSession() middleware in middleware/apikey.go
    returns 403 with code apikey.user_session_required when
    CtxKeyAPIKeyScopes is set on Locals - i.e. the request came
    in with an API-key bearer.
  - server/router.go wraps GET/POST/DELETE /api-keys in an
    authed.Group("/api-keys", RequireUserSession()). JWT bearers
    still work, API-key bearers get 403 regardless of payload.

Why not subset-check the scopes (approach 2)? Because that still
lets an API key manage credentials, which means a leaked key can
*revoke* the legitimate owner's other keys (denial of service) and
rotate the leaked key itself (evading the owner's revoke). Strict
"no credential management from API keys" is the simpler and more
auditable contract; future admin tooling that legitimately needs
it can use a JWT-exchange flow rather than a scope.

Tests:
  - TestRequireUserSession_RejectsAPIKeyAuth: covers both the
    reject (API-key bearer → 403) and pass-through (JWT bearer →
    200) paths. Regression for the escalation vector.

go test -race ./internal/... clean, golangci-lint clean.

Co-Authored-By: dede febriansyah <febriansyahd65@gmail.com>
dedeez14 added a commit that referenced this pull request Apr 25, 2026
…guard

fix(apikey): block API-key auth from /api-keys routes (post-#16 hotfix)
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