feat(agent-vault): enforce method and path rules, inject custom headers, substitute placeholders - #397
feat(agent-vault): enforce method and path rules, inject custom headers, substitute placeholders#397saifsmailbox98 wants to merge 19 commits into
Conversation
…stitute placeholders The resolve payload now carries a service's allowed methods and path prefixes, its extra headers, and its placeholder substitutions. The policy check runs on the request exactly as it arrived, above the plaintext refusal so a rule holds on http:// too, and answers a violation with a 403. Path matching never decodes: it compares the prefix against EscapedPath byte for byte at a segment boundary, and refuses any path carrying a . or .. segment, an empty segment, a ; or \, or an escape decoding to one of those, to a control byte, or to bytes that are not valid UTF-8. That last test is what lets a real non-ASCII path through while still refusing the overlong %c0%ae. Substitutions run before the credential so an injected value can never itself be rewritten, and custom headers last. A path substitution rewrites the path after it was authorised, so a path-restricted service re-checks what actually goes on the wire; that refusal carries no path, because by then the path holds the real credential and the error text is both the 403 body and the log line. The log gains a substituted field naming the surfaces actually rewritten, since the logged path is always the agent's own and a placeholder that matched nothing would otherwise look identical to one that fired.
…query value A path substitution rewrote the decoded Path and cleared RawPath, so Go re-derived the wire path from it. Its encoder does not re-escape '/' or '+', so a GitLab project addressed as group%2Fproject arrived at the upstream as two path segments, addressing a different repository. The swap now runs against EscapedPath and writes the result back into RawPath, and the replacement is escaped so a secret containing a slash cannot add a segment of its own. RawQuery goes on the wire verbatim and had the same bug: a base64 key containing '+' reached the upstream as a space, and one containing '&' split into a second parameter. Escaped now. packages/agentproxy carries the same two lines and is deliberately left alone.
|
💬 Discussion in Slack: #pr-review-cli-397-feat-agent-vault-enforce-method-and-path-rules-inject-headers Posted by Review Police — reviews, comments, new commits, and CI failures will stream into this channel. |
 |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
…apes, give the credential precedence
A placeholder carrying a character Go percent-escapes in a path never matched.
EscapedPath re-encodes the whole path whenever what the agent sent is not
already valid encoding, so a `{{TOKEN}}` on the wire reads as `%7B%7BTOKEN%7D%7D`
and the swap looked only for the form the author typed. The same placeholder
worked in the query string and silently did nothing in the path, and the agent
saw a third-party 404 with no warning. The swap now looks for the encoded form
too, produced by the same encoder EscapedPath falls back to.
The post-substitution path re-check refused the escaping applySubstitutions
itself writes. The replacement is PathEscaped precisely so a secret containing a
slash cannot add a segment, and that `%2F` is what isAmbiguousPath reads as a
separator: a GitLab project addressed as group%2Fproject answered 403 against a
prefix that plainly covered it, and the refusal blamed the path rule. The
re-check keeps the byte-exact prefix comparison, which is what holds the
substituted span after the prefix, and refuses only traversal, judged on the
decoded path because an upstream that decodes %2F before routing is the reader
`..%2F..%2Fadmin` is written for.
Custom headers are written before the credential rather than after, reversing
the order the previous commit set. One naming the credential's own header now
loses to it instead of replacing the real token with a custom value the agent
cannot explain. The backend refuses that pairing on write, so this is the floor
under that check rather than a replacement for it: a service saved before the
check existed, or a write that raced it, can no longer cost an agent its
credential. Pass-through injects nothing, so a custom Authorization header on
one still lands.
Follows the backend rename. The resolve payload field is `customHeaders`, so an older proxy reading `headers` finds nothing and a service's custom headers stop being attached until the binary is updated; the product is in preview and the field has no other consumer. AgentVaultHeader / json:"headers" -> AgentVaultCustomHeader / json:"customHeaders" resolvedService.headers -> resolvedService.customHeaders injectHeaders -> injectCustomHeaders stripHopByHopHeaders, req.Header and every other genuine HTTP-header name is left alone: this is only the name of the service's own configured headers.
![veria-ai[bot]](https://avatars.githubusercontent.com/in/1723776?s=60&v=4){kind=small}
PR overviewThis pull request extends Agent Vault proxy policies with method and path enforcement, custom header injection, and placeholder substitution in proxied requests. Three issues have been addressed, but four security concerns remain open. The most significant risks are credential exposure when secret-bearing requests use plaintext HTTP and method-policy bypasses that can let authenticated callers trigger disallowed upstream actions. A further placeholder-expansion flaw can crash affected 32-bit proxy builds under specific configurations. Open issues (4)
Fixed/addressed: 3 · PR risk: 7/10 |
…bel it A body read that failed partway was forwarded with ContentLength corrected to what had been read, which hands the upstream a well-formed shorter request it cannot tell from a complete one. Verified against a real transport: a body breaking at 300 of 1007 bytes arrived as a clean 300 byte request and the upstream answered 200, so a broken upload became a partial write nobody can take back. The declared length is left alone now, so the two disagree and http.Transport refuses the request outright. The upstream receives nothing and the agent gets a 502 it may not even be alive to see, which is the better end of that trade. The comment defending the old behaviour weighed the 502 against "the unchanged body this path promises", but the path cannot promise an intact body once the stream has broken. Only reachable on a service carrying a body substitution; everything else never reads the body at all.
The post-substitution path re-check refused a bare `..` segment and nothing else, which is narrower than the pre-check it replaced for a restricted service. `..` is not the only way up: Tomcat and Jetty strip `;params` per segment and IIS reads '\' as a separator, so with prefix /repos an agent sending `/repos/..__P__/admin` walked out to /admin on those upstreams whenever the substituted value began with '\' or ';'. Verified both, and that `..%2F` was already caught. ';' and '\' are refused on the decoded path now. The escape-shape checks isAmbiguousPath runs are still deliberately not repeated here, since the substituted span is percent-escaped on the way out and running them on the decoded form would refuse a value merely containing a '%'. The body path measured after reading, so a request over the 10 MiB cap cost the cap in memory before being refused, and maxConcurrentConns is 512. A declared length over the cap now skips the read entirely. The check after reading still stands on its own: a chunked request declares -1, and a declared length is a claim rather than a fact.
|
@greptile please re-review. Since the last pass:
The method case-folding comment was answered inline as a false positive: the allowed set is uppercase, so no casing reaches a method the policy did not list. Companion server PR: Infisical/infisical#8130 |
|
@greptile review |
`/repos` not covering `/repositories` was pinned; `/repos/octo` not covering `/repos/octopus` was not. It was the one case the TypeScript matcher's test had that this file did not, and that matcher is being removed as a copy of a rule that only runs here.
CLAUDE.md asks for no comments by default, and one earns its place only by explaining why: a non-obvious constraint, an ordering dependency, or logic that looks wrong until you know the reason. This branch added 160 comment lines to packages/agentvault, most of them retelling the line below. Gone: the doc comment restating a sentinel's declaration, the four-line account of where applySubstitutions was ported from, the seventeen-line essay above pathAllowedAfterSubstitution, and the fixture preamble describing its own return value. Kept, shorter: why ';' and '\' are traversal on Tomcat and IIS, why the UTF-8 check beats refusing every high byte, why ContentLength is left disagreeing, why the credential is injected last. 160 lines down to 92.
CLAUDE.md asks for no comments by default, and one earns its place only by explaining why: a non-obvious constraint, a workaround, an ordering dependency, or logic that looks wrong until you know the reason. This branch had 379 comment lines across 4,600 added ones, and most of them failed that test. Gone: every "null means unrestricted" beside a `| null` type, the notes telling a test what its own name already says, the accounts of which query runs where, the fixture preamble describing its own return value, and the paragraphs explaining an optimisation nobody would undo by accident. Kept, and shortened: why ';' and '\' count as traversal on Tomcat and IIS, why the UTF-8 check beats refusing every high byte, why ContentLength is left disagreeing with the body, why the credential is injected last, why removal is Base UI's and adding is ours, why the shadow check reads both halves on the primary, and why the rows are read before the delete that cascades them away. 379 down to 180.
saifsmailbox98
changed the title
…filled Header substitutions run before the body pass, so a request carrying Content-Encoding: <placeholder> and a body reached this warning with the real secret in it. The agent never held that secret and can read the proxy's own log, so only the presence of an encoding is recorded now.
| } | ||
|
|
||
| func requestPath(req *http.Request) string { | ||
| path := req.URL.EscapedPath() |
There was a problem hiding this comment.
Go’s EscapedPath() has a trap. If the path contains any byte Go thinks should have been encoded ({, }, |, ^, backtick, quote), it throws away what the agent actually sent and rebuilds the path from the decoded version, which turns %2F into a real /. hasUnsafeEscape exists specifically to refuse %2F. Adding one { anywhere in the path defeats it, because the %2F is already gone by the time the check runs.
There was a problem hiding this comment.
right, and worse than the %2F part alone — the path we forwarded wasn't the one the agent sent. tested it on the wire with a raw dial, service pinned to /repos:
/repos/a%2Fb -> 403
/repos/a%2Fb/{x} -> 200, upstream got /repos/a/b/%7Bx%7D
/repos/a%2F..%2Fadmin/{x} -> 403
so the agent asked for one segment a%2Fb and github got two, with the credential on it. traversal was still caught by the .. and // checks on the rebuilt path, so it wasn't a prefix escape, but "we broker to a different resource than was asked for" is bad enough on its own.
couldn't just refuse a brace, since {{PAT}} is the placeholder syntax and that's the main path through this feature. so the bytes Go objects to get escaped before anything reads the path, which keeps RawPath valid and stops the rebuild happening at all.
two things worth knowing about the fix. it only runs where Go was going to rebuild anyway — !, (, ), *, [, ] are rejected by shouldEscape but accepted by validEncoded, so escaping those would have changed the wire and broken a byte-compared prefix for no reason. and the predicate is derived from the stdlib rather than copied out of it, because the table behind encodePath is generated and a hand copy would drift a release later.
the wire does change in one place, deliberately: a percent-triple the agent wrote is now carried through instead of decoded, so %41 survives the same way %2F does. that's the fix rather than a side effect.
same bug at rewrite.go, fixed by the same thing since it reads the corrected value.
|
|
||
| stripHopByHopHeaders(resp.Header) | ||
| dst := w.Header() | ||
| for name, values := range resp.Header { |
There was a problem hiding this comment.
The proxy swaps the placeholder for the real token on the way out, and nobody checks the response on the way back. The agent asks for /repos/{{PAT}}, the upstream answers with the near-universal “add a trailing slash” redirect containing the full URL, and this loop copies that header back verbatim. The agent now holds the raw credential. Grep for scrub/redact across the package returns nothing, so the absence of any scrubbing is certain; whether a given upstream echoes depends on the upstream.
There was a problem hiding this comment.
reproduced it, and it's as bad as you'd expect — an ordinary 301 is enough, no hostile upstream needed. substitution value ghp_SUPERSECRET:
Location: http://.../repos/ghp_SUPERSECRET/
X-Request-Path: /repos/ghp_SUPERSECRET
body: {"error":"not found: /repos/ghp_SUPERSECRET"}
not changing it here though. a substitution into a path or a query puts the secret into a URL, and that's the admin's call at configuration time — the upstream logs it, proxies on the path see it, and getting it back in a redirect is the same exposure they already took on. we modify the request and we don't touch the response, and I'd rather that stay one simple rule than become "we scrub some of the ways it can come back".
worth knowing neither agentproxy nor the open-source agent-vault scrubs either, so it isn't a regression against the siblings.
if we do want to close it later, header scrubbing catches the redirect case cheaply and the body needs a rolling scan over the stream, gated on the request having actually substituted. not for this PR.
There was a problem hiding this comment.
Custom headers and substitutions sit inside the same https-only guard as the credential. A pass-through service whose only purpose is adding a header does nothing over http, and the single warning says the proxy refused to attach a credential, for a service that has no credential. Method and path rules run above that guard, so half the policy applies over http and half silently does not.
There was a problem hiding this comment.
you're right, and I went further than moving the two out of the guard — the guard's gone entirely, so a credential attaches over http the same as over https.
the reason it existed doesn't really survive contact: refusing doesn't make anyone safer, it just breaks the feature and pushes the admin to stick the real credential in the agent's env instead, which is permanent and is the thing this product exists to stop. agent-proxy and the open source agent-vault both inject regardless of scheme, so we were the odd one out.
what keeps a credential off a plaintext wire now is the portless-means-443 rule — a bare host never matches port 80, so a service only brokers over http if someone typed the port. felt like the right place for it: the config then says on its face that the credential goes out unencrypted.
worth saying what it doesn't stop: an agent that opens a tunnel to a plaintext port and speaks cleartext inside gets the credential in the clear. deliberate, and both siblings do the same.
the log line's fixed too, and there was a second thing hiding behind it — nilling the match also dropped the service and bundle name from the request log, so a plaintext request to a configured service showed up as if nothing had matched. it's an info line with plaintext=true now rather than a warning, since everything else we warn about is a failure or a refusal, not a config working as written.
tested live against a real service with a bearer credential, a custom header and a path substitution over plain http. before: credential NONE, placeholder untouched, no header. after: all three land.
| // pathAllowed would refuse that very '%2F'. Only traversal can leave an allowed prefix, so only traversal is | ||
| // refused, judged on the decoded path because that is what an upstream decoding '%2F' will route on. ';' and | ||
| // '\' count as traversal here for the reason isAmbiguousPath gives. | ||
| func pathAllowedAfterSubstitution(escaped, decoded string, prefixes []string) bool { |
There was a problem hiding this comment.
Two functions check the same path and disagree, and the weaker one guards the path that already carries the real secret. Wider than I first filed it; all four of these are refused before substitution and accepted after:
/repos//admin pre=false post=true
/repos/%00admin pre=false post=true
/repos/%09admin pre=false post=true
/repos/%C0%AE%C0%AE/admin pre=false post=true
An empty vault value turns /repos/PAT/admin into /repos//admin, and nothing validates that a substitution value is non-empty. The maintenance half matters too: lines 67 and 70-74 are hand-copies of 96 and 102-106, so hardening the first check silently skips the second.
There was a problem hiding this comment.
valid, and wider than the four you listed in one direction and narrower in another.
wider: the two functions disagree because the second one is weaker on purpose, and that wasn't written down. we PathEscape the substituted value so it can't add a segment, and isAmbiguousPath refuses that very escaping — I tried unifying them and it 403s our own output:
secret "org/repo" -> /repos/org%2Frepo/admin -> refused
secret "a%b" -> /repos/a%25b/admin -> refused
so they have to stay separate. what the second one was missing is the shapes a value can introduce, judged on the decoded path where our own escaping is invisible. //, control bytes and invalid UTF-8 are refused there now, and ghp_plain, org/repo, v1.2, a%b all still pass.
narrower: your empty-value trigger doesn't fire. secretValueSchema is .min(1) and blocks control chars, and the agent's half of the path has already been through isAmbiguousPath on arrival, so none of the four shapes can actually be produced today. it was latent, held shut by validation in the other repo, which isn't something this package should be leaning on — hence fixing it anyway.
one behaviour change worth flagging: a secret with a leading or trailing slash now fails this check rather than reaching the upstream as a doubled separator. /admin in /repos/{{PAT}}/x used to go out as /repos//admin/x. tested.
and yes on the maintenance half — the comment now says why the two differ, so it doesn't read as a hand-copy that drifted.
| path := req.URL.EscapedPath() | ||
| if path == "" { | ||
| // OPTIONS * arrives as "*" and is left alone; a genuinely empty path is the root. | ||
| if req.URL.Opaque != "" { |
There was a problem hiding this comment.
http:admin/secrets through the tunnel gives Go an empty path and a non-empty Opaque. The plain-HTTP handler rejects that shape, but the tunnel handler calls forwardHTTP directly and nobody checks. With no prefixes configured (the default) it goes upstream as GET admin/secrets HTTP/1.1, no leading slash, with a real Authorization header. Also, the comment on line 37 about OPTIONS * is wrong: Go answers that itself and never calls the handler.
There was a problem hiding this comment.
you're right that nobody checks it, and the shape reaches forwardHTTP through the tunnel while the plain door turns it away — though by way of the empty host rather than any opaque check, so the two doors disagreed by accident rather than by design. refused in forwardHTTP now, where both of them meet.
one thing it isn't, though: an access bypass. with no prefixes configured the service allows every path anyway, so the agent could have sent /admin/secrets normally and got the same credential. host pinning also holds, so it can't reach anywhere else.
what it actually cost is the record. forwardHTTP logged r.URL.EscapedPath(), which is empty for an opaque target, so the policy check saw admin/secrets, the upstream saw admin/secrets, and the log line showed nothing. the one request shape that looks deliberate was the one you couldn't see afterwards. the logged path comes from requestPath now, which can't be blank.
and the OPTIONS * comment is wrong, as you said — checked the stdlib, globalOptionsHandler takes it before our handler runs, for both the front server and the per-tunnel one. it's misplaced as well as stale: GET * parses to Path "*", which isn't empty, so the branch it annotates was never reachable for * either way. gone.
| return nil, nil, outcome, fmt.Errorf("%w: %w", errSessionResolve, err) | ||
| } | ||
|
|
||
| matched := bestMatch(services, hostname, port) |
There was a problem hiding this comment.
Service selection looks only at host and port, and on a policy failure it gives up instead of trying the next service. With api.github.com + /repos (read token) and api.github.com + /admin (admin token), whichever wins the host lookup handles every request and the other credential is unreachable behind a 403 that blames the path rule. packages/agentproxy/match.go:121 already threads the path into its matcher for this reason.
There was a problem hiding this comment.
I don't think this one can happen, at least not yet.
two services on one host in one bundle is a hard reject at write time — tried building your exact example on a dev stack:
POST .../services name=gh-read api.github.com /repos -> created
POST .../services name=gh-admin api.github.com /admin -> 400
"'gh-read' already covers api.github.com:443 in this access bundle."
the conflict rule is host-only on purpose: methods and prefixes are filters on a service that already matched, not part of the match key. across bundles it's allowed, but AGENT_VAULT_MAX_SESSION_BUNDLES is 1, so a session carries one bundle and the two never meet.
the agentproxy citation is right but the reason differs — it allows overlapping services, so it has to disambiguate at request time. we forbid the overlap instead, and that's what makes host+port selection safe here.
it does become real the day multi-bundle sessions ship, since two bundles could each cover the same host and first-match would quietly pick one. worth remembering as a prerequisite for that rather than a fix now.
The empty check ran before the trailing-slash trim, so "//" passed it and then became "", which matches every path. A service restricted to /repos allowed /admin instead, the opposite of what the comment above promises. The server rejects "//" on write, so this was never live, but the proxy is the enforcement point and revalidates nothing it is sent.
Substitutions ran in the order the server sent them, so a placeholder starting with another one ate its prefix: with __TOKEN__ before __TOKEN__V2, the header meant for the second secret went out carrying the first with V2 glued on, and the second never sent. The same pair was correct in the other order. Sorted at resolve time rather than per request, since the slice is shared by every request a session serves.
A client that builds the query from parameters rather than a string
percent-encodes first, so {{TOKEN}} arrives as %7B%7BTOKEN%7D%7D and the
literal match missed it. The placeholder then reached the third party and
the 401 that came back explained nothing.
The path surface already falls back to the escaped form; this is the same
fallback with query escaping. Underscore-style placeholders are never
encoded and were never affected.
…ing space is dropped hasUnsafeEscape refuses control bytes with "< 0x20", and a space is exactly 0x20, so "%20" walked past it. Windows and IIS drop a trailing space or dot from a segment, so "/repos/..%20/admin" reads here as an ordinary segment named "..%20" and arrives there as "..". A service pinned to /repos reached /admin with the credential attached. "...." needed no encoding at all, since the segment check only matched "." and ".." exactly. Judged on the decoded segment rather than by widening the byte rule. Refusing every 0x20 would have taken "/repos/my%20repo" with it, which is a real path and is already pinned as allowed.
… so in the log TRACE and TRACK make the upstream reflect the injected credential back in the response body, so both are refused. The comparison was exact while allowsMethod had always upper-cased, and Go passes the method through untouched, so a lowercase "trace" walked past. The policy check caught it wherever a service restricted methods; the gap was services restricting none, which is the default. The refusal also returned straight out of the handler, ahead of the logging the other refusals go through, leaving the one refusal that means somebody reached for the credential with no record. Raised inside forward now, so it reads as errPolicyBlocked like the rest: 403 rather than 405, and logged.
…estricts methods Rails, Laravel and Symfony treat X-HTTP-Method-Override and its two siblings as the method to perform, so a service restricted to GET and POST was one header away from a DELETE: the allowlist read POST off the wire, allowed it, attached the credential, and the framework did the rest. The path checks in this package already anticipate what an upstream does to a request after it arrives, semicolons on Tomcat and backslashes on IIS among them. The method check assumed the wire method was the one that runs. Only where a service restricts methods. With no restriction the agent can send DELETE outright, so the header buys it nothing and there is no control to protect.
Encoded placeholders are matched whatever case their escapes use. Percent -escapes carry no required case and clients differ, but the fallback built its needle upper-cased and compared byte for byte, so `%7b%7bPAT%7d%7d` never matched and no substitution happened. Both surfaces had it; the query fallback landed yesterday carrying the same flaw. Normalising escape case costs nothing in the branch that is about to rewrite the URL anyway, since substituting invalidates any signature the agent computed itself. A substitution that matches nothing now says so. The failure was a miss rather than a leak, the placeholder going upstream and the agent seeing a third-party 401 it cannot explain, but it was silent, which this file avoids everywhere else. Body substitutions are judged where they are applied and stay out of it. Query values escape per RFC 3986 rather than as form data. QueryEscape turns a space into '+', which anything reading the raw query takes literally, SigV4 signing among them. The path surface three lines up already had this right, and the test could not have caught the difference, asserting through ParseQuery, which turns '+' back into a space. It compares the wire query now. The expansion limit compares by division. count*(len(new)-len(old)) overflows int on the 386 and armv6 builds goreleaser ships and wraps negative, so the guard answers "small enough" for a request that then fails to allocate. A 10MB body of a five-character placeholder against a 1.2KB secret is enough, and the agent picks the body while the secret comes from the service.
The guard refused everything over http, not just the credential: a pass-through service whose whole job is adding a header did nothing at all, and the log line announced a refusal to attach a credential it never had. Both sibling products inject regardless of scheme, agent-proxy with no check at all and the open-source agent-vault through a purpose-built http path, so this was the only one of the three that refused. Refusing made nobody safer. The fallback it pushed an admin to is putting the real credential in the agent's environment, where it sits permanently, which is the thing the product exists to prevent. An API reachable only over http is ordinary inside a network, and running it that way is the admin's call on their own network. A portless pattern still means 443, and that is now the whole of what keeps a credential off a plaintext wire: injection is scheme-blind, so naming a port is how a service opts in, and the config then says on its face that a credential goes out unencrypted. What it does not defend against is an agent that opens a tunnel to a plaintext port and speaks cleartext inside it, which is deliberate and matches both siblings. Nilling the match also stripped the service and access bundle from the request log, so a plaintext request to a configured service was recorded as if nothing matched it. The line carries the service again, and a brokered request over http is tagged rather than warned about: every other warning in the package is a failure, a refusal or a limit, never a config working as written.
| injectCredential(req, &matched.credential) | ||
| // Substitutions first, so an injected real value can never itself be rewritten. The credential last, | ||
| // so a custom header naming the credential's own header loses rather than replacing the token. | ||
| outcome.substituted = applySubstitutions(req, matched.name, matched.substitutions) |
There was a problem hiding this comment.
Medium: Credentials sent over plaintext HTTP
An on-path attacker can capture the injected credential, secret custom headers, or substituted values whenever a service pattern explicitly names a plaintext HTTP port, because all three injections now run regardless of scheme. Restore the HTTPS requirement for secret-bearing values; pass-through services that only add non-sensitive headers can be handled separately if plaintext support is required.
| // Rails, Laravel and Symfony all honour these, so a POST carrying one performs the method it names. The | ||
| // wire method is what the allowlist judged, so where there is an allowlist the header has to go. | ||
| func stripMethodOverrideHeaders(header http.Header) { | ||
| for _, name := range []string{"X-HTTP-Method-Override", "X-Method-Override", "X-HTTP-Method"} { |
There was a problem hiding this comment.
Medium: Form method override bypasses the allowlist
An authenticated proxy caller can send an allowed POST containing _method=DELETE in form data and have a Rails upstream execute the disallowed method; Rails forms use this hidden parameter for non-GET/POST methods. Deleting override headers does not inspect the query or request body, so the policy still authorizes only the wire method. Reject method-override parameters or derive and enforce the effective method before forwarding. citeturn0search0
EscapedPath rebuilds a path from its decoded form whenever RawPath is not valid
encoding, and one literal '{' is enough to trigger it. The rebuild turns '%2F'
into a real '/', so hasUnsafeEscape never saw the escape it exists to refuse:
'/repos/a%2Fb' was a 403 and '/repos/a%2Fb/{x}' was a 200 that reached the
upstream as '/repos/a/b/...', two segments where the agent sent one, with the
credential attached. Braces cannot simply be refused, since the placeholder
syntax is made of them, so the bytes Go objects to are escaped before anything
reads the path and RawPath stays valid.
The rewrite runs only where Go would have rebuilt anyway, which is what keeps
it from touching a path Go already accepts: '!', '(', ')', '*', '[' and ']' are
rejected by shouldEscape but accepted by validEncoded, so escaping them would
change the wire and break a byte-compared prefix. Within that narrower set the
output is Go's own rebuild, except that a percent-triple the agent wrote is
carried through rather than decoded. That difference is the fix, so the wire
does change for '%41' as much as it does for '%2F'; what does not change is the
brace class, which Go was already sending escaped. The predicate is derived
from the standard library rather than transcribed, because the table behind
encodePath is generated and a copy would drift.
A body that cannot be read whole is refused rather than forwarded short. The
old safety net left ContentLength disagreeing with the bytes so the transport
would refuse, but a chunked upload declares -1 and there is nothing to leave
wrong, so the upstream received a well-formed partial request carrying the
credential, could not tell it was partial, and answered 200 while the log said
we had refused to forward a truncated one. Only that read failure is an error;
a skipped body, an encoded one and an oversized one stay the no-op they were.
The path and headers are rewritten before the body is read, so the surfaces
that fired are returned alongside the error: the request is refused, but the
record still has to say the credential was written into it. It is the agent's
own upload that broke, so it earns a 400 rather than the upstream's 502.
An opaque request target is refused. 'http:admin/secrets' parses to an empty
path and a non-empty Opaque; the plain door already turned it away, though by
way of the empty host rather than the shape, and the tunnel reached forwardHTTP
directly, so with no path prefixes configured it went upstream as 'GET
admin/secrets HTTP/1.1' with a real credential. Not an access bypass, since a
service with no prefixes allows every path anyway, but the request was logged
with an empty path, so the one shape that looks deliberate was the one the
record could not show. The logged path comes from requestPath now, which cannot
be blank. The refusal sits above the path rewrite so an opaque target cannot
acquire a synthesized path on its way to being refused.
The post-substitution path check refuses what a substituted value can
introduce. It is weaker than isAmbiguousPath on purpose, because the value is
escaped by us and isAmbiguousPath refuses that very escaping, so a secret like
'org/repo' would be rejected on its own '%2F'. It judged only ';', '\' and dot
segments, which left '//', control bytes and an overlong '..' refused on
arrival and accepted once the real secret was in the path. Those three are
judged on the decoded form, where our own escaping is invisible. A secret
carrying an edge slash now fails that check rather than reaching the upstream
as a doubled separator, which is a behaviour change for a value a deployment
could hold.
| if err != nil { | ||
| return nil, matched, outcome, err | ||
| } | ||
| outcome.brokered = injectCustomHeaders(req, matched.customHeaders) |
There was a problem hiding this comment.
Low: Injected headers bypass the method allowlist
Method-override headers are stripped before custom and credential headers are injected. For a restricted service configured to inject X-HTTP-Method-Override, X-Method-Override, or X-HTTP-Method, a session holder can send an allowed POST and make a supporting upstream execute the injected disallowed verb. Strip these headers again after all brokered headers are added, or reject these header names when resolving a method-restricted service.
| if count == 0 { | ||
| continue | ||
| } | ||
| if len(rewritten)+count*(len(sub.value)-len(sub.placeholder)) > maxBodyRewriteSize { |
There was a problem hiding this comment.
Low: Body expansion can crash 32-bit proxy builds
This multiplication can overflow int on the published 386 and armv6 builds. An authenticated caller can fill a request body with a short configured placeholder so the wrapped size passes this check, after which bytes.ReplaceAll attempts the oversized expansion and can crash the shared proxy process. Use the division-based growth check already implemented by replaceWithinLimit.
| // first byte decides: a TLS record starts with 0x16, anything else is plain HTTP and takes the same path | ||
| // as absolute-form, which already refuses to attach a credential over plaintext. | ||
| // as absolute-form. The port stays the one the CONNECT line named, so a service still only matches if | ||
| // its pattern covers that port. |
There was a problem hiding this comment.
The port opt-in doesn't hold on this path. An agent that CONNECTs to a service's ordinary port and then speaks plaintext instead of handshaking gets the credential attached in the clear, with no plaintext port ever configured.
Just below, anything whose first byte isn't 0x16 goes to serveTunnel with scheme "http" and the port from the CONNECT line. For any ordinary target that port is 443, which is exactly what a service that never named a port defaults to via defaultPort. So bestMatch matches on 443, and with the https guard gone forward injects into a plaintext request.
Drove it end to end through the real proxy. hostPatterns hand-built as {host, port, portWritten: false}, which is the shape a service that never named a port gets, with the fixture's port standing in for 443 so the forward could actually land. Nothing else on the path differs:
CONNECT -> 200 Connection Established
{"decision":"brokered","status":200,"service":"github","plaintext":true,...}
UPSTREAM SAW: path="/repos/x" Authorization=[Bearer tok_real]
In production that is CONNECT api.github.com:443, then a plaintext GET inside the tunnel, and the real token goes onto a TCP socket in cleartext. A TLS-only upstream won't answer it, but the bytes have already left the machine.
The plaintext: true log field is a good addition and means this is at least auditable. What's missing is the enforcement. proxy_plaintext_test.go states the invariant as "A service reaches this fixture only by naming its port, which is what opts it into plaintext", and all three of its cases drive the absolute-form leg, so nothing pins the tunnel.
Two small ways to close it: refuse a non-TLS first byte inside a tunnel outright, or gate plaintext brokering on portWritten rather than on port equality, so a service sitting on the 443 default can't be reached this way.
2 participants
Description 📣
Proxy side of method and path rules, custom headers and substitutions. The API hands the resolved service its
allowedMethods,allowedPathPrefixes,customHeadersand substitutions, and the proxy enforces them.A request outside a service's methods or paths gets a 403 with
blocked by service policy, rather than being forwarded without its credential. The check runs on the request as it arrived, before anything rewrites it.Path prefixes match whole segments and are compared against
EscapedPath(), never a decoded one. A path-restricted service refuses outright anything that would have to be normalised to judge, which is what stops/admin/%2e%2e/repos/xreading as two different paths to us and to the upstream. A nil methods map means every method, so an unrestricted service is unaffected.Substitutions run on the escaped path and write back escaped, and the replacement is escaped too, so a secret containing a slash cannot add a segment and a project addressed as
group%2Fprojectdoesn't arrive as two segments.RawQueryhad the same shape of bug: a base64 key containing+reached the upstream as a space.The placeholder is matched in the escaped form as well as the form the author typed.
EscapedPath()re-encodes the whole path whenever what the agent sent is not already valid encoding, so a{{TOKEN}}on the wire reads as%7B%7BTOKEN%7D%7D: matching only the literal left those placeholders on the wire, working in the query string and silently doing nothing in the path, with the agent seeing a third-party 404 and no warning.The post-substitution path re-check no longer refuses the escaping this file itself writes. The replacement is
PathEscaped precisely so a secret containing a slash cannot add a segment, and that%2Fis whatisAmbiguousPathreads as a separator, so a GitLab project addressed asgroup%2Fprojectanswered 403 against a prefix that plainly covered it, and the refusal blamed the path rule. The prefix comparison stays byte-exact, which is what holds the substituted span after the prefix, and the only thing still refused is traversal, judged on the decoded path because an upstream that decodes%2Fbefore routing is the reader..%2F..%2Fadminis written for.Injection order: custom headers are written before the credential, so one naming the credential's own header loses to it instead of replacing the real token with a value the agent cannot explain. The backend refuses that pairing on write, so this is the floor under that check rather than a replacement for it: a service saved before the check existed, or a write that raced it, can no longer cost an agent its credential. Pass-through injects nothing, so a custom
Authorizationheader on one still lands.The resolve payload field is
customHeaders, matching the server, so this and the server PR land together.Server PR: Infisical/infisical#8130
Type ✨
Tests 🛠️
# Here's some code block to paste some code snippets