feat(broker): restrict services to specific HTTP methods - #324
feat(broker): restrict services to specific HTTP methods#324cjohnhanson wants to merge 1 commit into
Conversation
Adds an optional methods list to a service. When present, the credential is attached only to requests using a listed method; any other method is denied with 403 method_not_allowed before either credential path (auth headers or substitutions) resolves. The check runs on the single MatchService winner after selection, parallel to IsEnabled — no fall-through to a broader sibling. Method-restricted services also have the X-HTTP-Method-Override / X-Method-Override / X-HTTP-Method headers and the _method query parameter stripped before forwarding. Omitted methods = all methods; an explicit empty list is rejected. Values are uppercase-normalized on ingest and compared case-sensitively at request time.
|
✅ CLA satisfied. All contributors have signed the current CLA. The |
|
| Filename | Overview |
|---|---|
| internal/broker/broker.go | Adds Methods []string field, NormalizeAndValidateMethods, and AllowsMethod. Validation rejects empty lists and duplicates, uppercases on ingest, and mutates via cfg.Services[i] not a range copy. Logic is correct and well-tested. |
| internal/brokercore/credential.go | Adds method parameter to Inject, inserts AllowsMethod gate after IsEnabled check (before credential resolution), and populates MethodRestricted in InjectResult. Method check correctly fires before any secret is accessed. |
| internal/mitm/forward.go | Passes r.Method to Inject; strips method-override headers and _method query param for restricted services. outURL.RawQuery is stripped before outReq is created (correct order). Minor: percent-encoded _method keys are not stripped; emit status for method_not_allowed relies on coincidental 403 default. |
| internal/brokercore/brokercore.go | Adds ErrMethodNotAllowed case to WriteInjectError with structured message extraction from *MethodNotAllowedError. Returns 403 consistent with other broker policy denials. |
| internal/brokercore/errors.go | Adds ErrMethodNotAllowed sentinel error following the same pattern as ErrServiceDisabled. |
| internal/brokercore/credential_methods_test.go | Tests allowed/denied method injection, confirms zero credential-store touches on denial, and covers passthrough+substitutions denial. Tests are thorough and well-structured. |
| internal/mitm/forward_methods_test.go | Unit table for stripMethodOverrideParam and an end-to-end proxy test confirming override header and _method param are stripped for restricted services and preserved for unrestricted ones. |
| docs/learn/services.mdx | Adds Restricting HTTP methods section with YAML example, validation rules, and important caveats (no fall-through, override stripping, WebSocket note). Documentation is accurate and thorough. |
Reviews (1): Last reviewed commit: "feat(broker): restrict services to speci..." | Re-trigger Greptile
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| func stripMethodOverrideParam(rawQuery string) string { | ||
| if !strings.Contains(rawQuery, "_method") { | ||
| return rawQuery | ||
| } | ||
| parts := strings.Split(rawQuery, "&") | ||
| kept := parts[:0] | ||
| for _, p := range parts { | ||
| key := p | ||
| if i := strings.IndexByte(p, '='); i >= 0 { | ||
| key = p[:i] | ||
| } | ||
| if key == "_method" { | ||
| continue | ||
| } | ||
| kept = append(kept, p) | ||
| } | ||
| return strings.Join(kept, "&") |
There was a problem hiding this comment.
%-encoded _method key bypasses stripping
stripMethodOverrideParam compares the raw key bytes against the literal string "_method". A query string such as %5Fmethod=DELETE has key %5Fmethod, so the check key == "_method" is false and the parameter is forwarded. Upstreams that decode percent-encoded parameter names before honoring _method (older Rails/Rack, some PHP frameworks) would still see an effective method override on a method-restricted service, provided the outer HTTP method is in the allowed list.
| case errors.Is(err, brokercore.ErrMethodNotAllowed): | ||
| errCode = "method_not_allowed" | ||
| } | ||
| brokercore.WriteInjectError(w, err, target, scope.VaultName, p.baseURL) | ||
| emit(status, errCode) | ||
| return |
There was a problem hiding this comment.
emit carries stale status for ErrMethodNotAllowed
When ErrMethodNotAllowed is matched, status stays at its default http.StatusForbidden (403). That coincidentally matches what WriteInjectError sends. But if WriteInjectError ever changes its response code for this error (e.g. to 405), emit would report the wrong code to the event/metrics sink. Setting status = http.StatusForbidden explicitly in the case, like the credential-missing case does for 502, removes the silent dependency between the two call sites.
Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
1 participant
Implements #323: an optional
methodsfield on a service so the credential is attached only to requests using the listed HTTP methods.methods: [GET, HEAD]brokers a privileged token read-only.What's in here
broker.ServicegainsMethods []string. Omitted = all methods (backward compatible). Validation rejects an explicitmethods: [], uppercases values on ingest, and checks them againstGET/HEAD/POST/PUT/PATCH/DELETE/OPTIONS.MatchServicewinner, parallel to the existingIsEnabled()check — a method-excluded winner is denied outright with no fall-through to a broader sibling. It runs at theInjectboundary before either credential path resolves, so a denied request gets the credential in no form: no auth headers, no substitutions (including onpassthroughservices).method_not_allowed(consistent with every existing broker-policy denial on the ingress) and a message listing the allowed methods.X-HTTP-Method-Override/X-Method-Override/X-HTTP-Methodheaders and the_methodquery parameter stripped before forwarding, so an allowed method can't stand in for a denied one. Query stripping preserves the surviving pairs byte-for-byte (no re-encoding), so signed URLs keep their signatures. The_methodform in a POST body is not inspected.docs/learn/services.mdxgains a "Restricting HTTP methods" section; the matching-rules note now points at it.Tests
broker: normalization, unsupported/duplicate/empty-list rejection,AllowsMethodcase-sensitivity fail-closed.brokercore: allowed method injects; denied method returnsErrMethodNotAllowedwith zero credential-store touches — including the passthrough-with-substitutions case.mitm: unit table for_methodquery stripping, plus an end-to-end proxy test asserting the upstream sees the override header and_methodparam stripped on a restricted service and untouched on an unrestricted one.Full suite passes (
go test ./...).Not in this PR
Web UI service form, agent proposal flow, and SDK typings don't know about
methodsyet — the field is additive, so they keep working; happy to follow up on those (and on whether override-stripping should be split out, per the issue) once the shape is agreed.