diff --git a/packages/agentvault/cache.go b/packages/agentvault/cache.go index b4f71ac7..09c401b8 100644 --- a/packages/agentvault/cache.go +++ b/packages/agentvault/cache.go @@ -39,12 +39,29 @@ type credential struct { password []byte } +type customHeader struct { + name string + prefix string + value []byte +} + +type substitution struct { + placeholder string + surfaces map[string]bool + value []byte +} + type resolvedService struct { id string name string accessBundleName string hostPatterns []hostPattern - credential credential + // A nil map means every method is allowed; an empty slice of prefixes means every path. + allowedMethods map[string]bool + allowedPathPrefixes []string + credential credential + customHeaders []customHeader + substitutions []substitution } type sessionEntry struct { diff --git a/packages/agentvault/match.go b/packages/agentvault/match.go index 6e5171cf..ab5eceff 100644 --- a/packages/agentvault/match.go +++ b/packages/agentvault/match.go @@ -5,8 +5,9 @@ import ( "strings" ) -// A pattern with no port covers every port in Agent Proxy's grammar, which lets plaintext port 80 -// through with the credential attached. Defaulting to 443 keeps that from happening here. +// A pattern with no port covers every port in Agent Proxy's grammar, which lets plaintext port 80 through +// with the credential attached. Defaulting to 443 keeps that from happening here, and is the whole of it: +// injection itself is scheme-blind, so naming a plaintext port is how an admin opts a service into it. const defaultPort = "443" // hostPattern carries no path: paths are rejected at write time, since the matcher would compare the @@ -15,7 +16,7 @@ type hostPattern struct { host string port string // Whether the entry named a port itself. Only the exception list reads this: a service without one - // has to stay on 443 or a credential would go out in the clear, but an exception carries no + // stays on 443, since that is what keeps a credential off plaintext, but an exception carries no // credential, so a bare host there means the host rather than one port of it. portWritten bool } diff --git a/packages/agentvault/policy.go b/packages/agentvault/policy.go new file mode 100644 index 00000000..e14ab32a --- /dev/null +++ b/packages/agentvault/policy.go @@ -0,0 +1,281 @@ +package agentvault + +import ( + "errors" + "fmt" + "net/http" + "net/url" + "strings" + "unicode/utf8" +) + +var errPolicyBlocked = errors.New("blocked by service policy") + +// The agent's own upload broke part way. Not a policy refusal and not an upstream failure, so it carries its +// own status rather than landing in either of theirs. +var errBodyUnreadable = errors.New("could not read the request body") + +func checkServicePolicy(svc *resolvedService, req *http.Request) error { + if !svc.allowsMethod(req.Method) { + return fmt.Errorf("service %q does not allow %s: %w", svc.name, req.Method, errPolicyBlocked) + } + if len(svc.allowedPathPrefixes) > 0 { + path := requestPath(req) + if !pathAllowed(path, svc.allowedPathPrefixes) { + return fmt.Errorf("service %q does not allow path %q: %w", svc.name, truncatePath(path), errPolicyBlocked) + } + } + return nil +} + +func (s *resolvedService) allowsMethod(method string) bool { + if s.allowedMethods == nil { + return true + } + return s.allowedMethods[strings.ToUpper(method)] +} + +// 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"} { + header.Del(name) + } +} + +// Whether Go would escape a byte appearing unescaped in a path. Derived from the standard library rather +// than transcribed from it: the table behind encodePath is generated, so a copy would be one Go release +// away from disagreeing with the rebuild this guards against. +var pathByteNeedsEscape = func() (table [256]bool) { + for b := 0; b < 256; b++ { + raw := string([]byte{byte(b)}) + table[b] = (&url.URL{Path: raw}).EscapedPath() != raw + } + return table +}() + +// EscapedPath falls back to rebuilding the path from its decoded form whenever RawPath is not valid +// encoding, and one literal '{' is enough. The rebuild turns '%2F' into a real '/', so hasUnsafeEscape +// never sees the escape it exists to refuse and the upstream receives a path the agent did not send. +// Escaping those bytes ourselves keeps RawPath valid, so EscapedPath returns it untouched. The wire form is +// unchanged either way: Go was already sending '%7B'. +func normalizeRequestTarget(u *url.URL) { + if u.RawPath == "" || u.EscapedPath() == u.RawPath { + return + } + escaped := escapeInvalidPathBytes(u.RawPath) + decoded, err := url.PathUnescape(escaped) + if err != nil { + return + } + u.Path = decoded + u.RawPath = escaped +} + +// A '%' opening a valid triple is carried through, so an escape the agent wrote is never escaped twice. A +// malformed one cannot arrive: ParseRequestURI rejects it and the server answers 400 before the handler. +func escapeInvalidPathBytes(raw string) string { + var out strings.Builder + out.Grow(len(raw)) + for i := 0; i < len(raw); i++ { + c := raw[i] + if c == '%' && i+2 < len(raw) { + if _, hiOk := unhex(raw[i+1]); hiOk { + if _, loOk := unhex(raw[i+2]); loOk { + out.WriteString(raw[i : i+3]) + i += 2 + continue + } + } + } + if pathByteNeedsEscape[c] { + const hexDigits = "0123456789ABCDEF" + out.WriteByte('%') + out.WriteByte(hexDigits[c>>4]) + out.WriteByte(hexDigits[c&0x0f]) + continue + } + out.WriteByte(c) + } + return out.String() +} + +func requestPath(req *http.Request) string { + path := req.URL.EscapedPath() + if path == "" { + // forwardHTTP refuses an opaque target before this runs, so the branch is a floor under that check + // rather than a shape expected here. A genuinely empty path is the root. + if req.URL.Opaque != "" { + return req.URL.Opaque + } + return "/" + } + return path +} + +func truncatePath(path string) string { + if len(path) > maxLoggedPathLen { + return path[:maxLoggedPathLen] + "...[truncated]" + } + return path +} + +// Never decodes: anything whose meaning depends on the upstream's normalisation is refused outright, so the +// comparison below is a plain byte comparison. +func pathAllowed(escaped string, prefixes []string) bool { + if isAmbiguousPath(escaped) { + return false + } + return matchesPrefix(escaped, prefixes) +} + +// Deliberately not isAmbiguousPath. The path here is part-written by us: applySubstitutions escapes the +// value so it cannot add a segment, and isAmbiguousPath refuses that very '%2F', so a secret like +// 'org/repo' would be rejected on its own escaping. Judged on the decoded path instead, which is both what +// an upstream decoding '%2F' will route on and the form our own escaping is invisible in. Everything below +// is a shape a substituted value could introduce; the agent's half of the path has already been through +// isAmbiguousPath on arrival. +func pathAllowedAfterSubstitution(escaped, decoded string, prefixes []string) bool { + if strings.ContainsAny(decoded, ";\\") { + return false + } + // Normalises differently per server, and an empty value substituted mid-path is how it arises here. + if strings.Contains(decoded, "//") { + return false + } + for i := 0; i < len(decoded); i++ { + if decoded[i] < 0x20 || decoded[i] == 0x7f { + return false + } + } + // '%c0%ae' is an overlong '.', which some servers read as a dot and route on. + if !utf8.ValidString(decoded) { + return false + } + for _, segment := range strings.Split(decoded, "/") { + if segment == "." || segment == ".." { + return false + } + } + return matchesPrefix(escaped, prefixes) +} + +func matchesPrefix(escaped string, prefixes []string) bool { + for _, prefix := range prefixes { + if prefix == "/" { + return true + } + if !strings.HasPrefix(escaped, prefix) { + continue + } + if rest := escaped[len(prefix):]; rest == "" || rest[0] == '/' { + return true + } + } + return false +} + +func isAmbiguousPath(escaped string) bool { + // Tomcat and Spring strip ;params before normalising, so /repos/..;/admin resolves to /admin upstream + // while reading as an ordinary segment here. IIS reads '\' as a separator. + if strings.ContainsAny(escaped, ";\\") { + return true + } + if hasUnsafeEscape(escaped) { + return true + } + for _, segment := range strings.Split(escaped, "/") { + if isDotSegment(decodeBenignEscapes(segment)) { + return true + } + } + // /a//b normalises differently per server. + return strings.Contains(escaped, "//") +} + +// The UTF-8 check is what lets a real non-ASCII path through while still refusing the attack: `%c0%ae` is +// an overlong '.', which some servers read as a dot, while `%c3%a9` is a legitimate 'é'. Refusing every +// byte >= 0x80 would catch the first and break every API carrying a filename in its path. +func hasUnsafeEscape(escaped string) bool { + decoded := make([]byte, 0, len(escaped)) + sawEscape := false + + for i := 0; i < len(escaped); i++ { + if escaped[i] != '%' { + decoded = append(decoded, escaped[i]) + continue + } + if i+2 >= len(escaped) { + return true + } + hi, hiOk := unhex(escaped[i+1]) + lo, loOk := unhex(escaped[i+2]) + if !hiOk || !loOk { + return true + } + b := hi<<4 | lo + if b < 0x20 || b == 0x7f { + return true + } + switch b { + case '.', '/', '\\', ';', '%': + return true + } + decoded = append(decoded, b) + sawEscape = true + i += 2 + } + + // Only escaped input can carry an overlong sequence. + return sawEscape && !utf8.Valid(decoded) +} + +// Runs after hasUnsafeEscape, so every escape still standing decodes to something harmless. Only the +// decoded form tells us whether a segment is all dots and spaces: "..%20" is not, ".. " is. +func decodeBenignEscapes(segment string) string { + if !strings.Contains(segment, "%") { + return segment + } + out := make([]byte, 0, len(segment)) + for i := 0; i < len(segment); i++ { + if segment[i] != '%' || i+2 >= len(segment) { + out = append(out, segment[i]) + continue + } + hi, hiOk := unhex(segment[i+1]) + lo, loOk := unhex(segment[i+2]) + if !hiOk || !loOk { + out = append(out, segment[i]) + continue + } + out = append(out, hi<<4|lo) + i += 2 + } + return string(out) +} + +// Windows and IIS strip trailing dots and spaces from a segment, so anything built only from those reads +// as "." or ".." once it lands. +func isDotSegment(segment string) bool { + if segment == "" { + return false + } + for i := 0; i < len(segment); i++ { + if segment[i] != '.' && segment[i] != ' ' { + return false + } + } + return true +} + +func unhex(c byte) (byte, bool) { + switch { + case c >= '0' && c <= '9': + return c - '0', true + case c >= 'a' && c <= 'f': + return c - 'a' + 10, true + case c >= 'A' && c <= 'F': + return c - 'A' + 10, true + } + return 0, false +} diff --git a/packages/agentvault/policy_test.go b/packages/agentvault/policy_test.go new file mode 100644 index 00000000..1d1ef43b --- /dev/null +++ b/packages/agentvault/policy_test.go @@ -0,0 +1,323 @@ +package agentvault + +import ( + "errors" + "net/http" + "net/url" + "strings" + "testing" +) + +func serviceWithPolicy(methods []string, prefixes []string) *resolvedService { + return &resolvedService{ + name: "github", + allowedMethods: toMethodSet(methods), + allowedPathPrefixes: toPathPrefixes(prefixes), + } +} + +func requestTo(t *testing.T, method, target string) *http.Request { + t.Helper() + req, err := http.NewRequest(method, "https://api.github.com"+target, nil) + if err != nil { + t.Fatalf("building request: %v", err) + } + return req +} + +func TestMethodPolicy(t *testing.T) { + t.Run("a nil set allows every method", func(t *testing.T) { + svc := serviceWithPolicy(nil, nil) + for _, method := range []string{"GET", "POST", "DELETE", "PROPFIND"} { + if err := checkServicePolicy(svc, requestTo(t, method, "/x")); err != nil { + t.Fatalf("%s should be allowed: %v", method, err) + } + } + }) + + t.Run("only the listed methods pass", func(t *testing.T) { + svc := serviceWithPolicy([]string{"GET", "HEAD"}, nil) + if err := checkServicePolicy(svc, requestTo(t, "GET", "/x")); err != nil { + t.Fatalf("GET should be allowed: %v", err) + } + err := checkServicePolicy(svc, requestTo(t, "POST", "/x")) + if !errors.Is(err, errPolicyBlocked) { + t.Fatalf("POST should be blocked, got %v", err) + } + if !strings.Contains(err.Error(), `service "github" does not allow POST`) { + t.Fatalf("unhelpful message: %q", err.Error()) + } + }) + + t.Run("a lower-case method is folded rather than blocked", func(t *testing.T) { + svc := serviceWithPolicy([]string{"GET"}, nil) + req := requestTo(t, "GET", "/x") + req.Method = "get" + if err := checkServicePolicy(svc, req); err != nil { + t.Fatalf("get should fold to GET: %v", err) + } + }) +} + +func TestPathPolicy(t *testing.T) { + svc := serviceWithPolicy(nil, []string{"/repos"}) + + allowed := []string{ + "/repos/my%20repo", + "/repos/...name", "/repos", "/repos/", "/repos/octo/hello", "/repos/a%20b"} + for _, path := range allowed { + t.Run("allows "+path, func(t *testing.T) { + if err := checkServicePolicy(svc, requestTo(t, "GET", path)); err != nil { + t.Fatalf("%s should be allowed: %v", path, err) + } + }) + } + + blocked := []string{ + "/repositories", + "/repo", + "/admin", + "/repos/../admin", + "/repos/./x", + "//repos/x", + "/repos/%2e%2e/admin", + "/repos/%2E%2E/admin", + "/admin/%2e%2e/repos/x", + "/repos/%252e%252e/admin", + "/repos/%c0%ae%c0%ae/admin", + "/repos/..;/admin", + "/repos;x/y", + "/repos/%2fadmin", + "/repos%5cx", + // Windows and IIS drop a trailing space or dot, so each of these lands as ".." upstream. + "/repos/..%20/admin", + "/repos/.. /admin", + "/repos/..../admin", + "/repos/.%20./admin", + } + for _, path := range blocked { + t.Run("blocks "+path, func(t *testing.T) { + req := requestTo(t, "GET", "/placeholder") + req.URL.Path = "" + req.URL.RawPath = "" + req.URL.Opaque = "" + parsed := requestTo(t, "GET", path) + req.URL = parsed.URL + err := checkServicePolicy(svc, req) + if !errors.Is(err, errPolicyBlocked) { + t.Fatalf("%s should be blocked, got %v", path, err) + } + }) + } + + t.Run("a backslash is blocked even where Go keeps it literal", func(t *testing.T) { + req := requestTo(t, "GET", "/repos") + req.URL.Path = `/repos/\..\admin` + req.URL.RawPath = "" + if err := checkServicePolicy(svc, req); !errors.Is(err, errPolicyBlocked) { + t.Fatalf("backslash traversal should be blocked, got %v", err) + } + }) + + t.Run("an unrestricted service is untouched by any of it", func(t *testing.T) { + open := serviceWithPolicy(nil, nil) + for _, path := range blocked { + req := requestTo(t, "GET", path) + if err := checkServicePolicy(open, req); err != nil { + t.Fatalf("%s should pass on an unrestricted service: %v", path, err) + } + } + }) + + t.Run("a deeper prefix still matches whole segments only", func(t *testing.T) { + deep := serviceWithPolicy(nil, []string{"/repos/octo"}) + if err := checkServicePolicy(deep, requestTo(t, "GET", "/repos/octo/hello")); err != nil { + t.Fatalf("/repos/octo should cover /repos/octo/hello: %v", err) + } + if err := checkServicePolicy(deep, requestTo(t, "GET", "/repos/octopus")); err == nil { + t.Fatal("/repos/octo should not cover /repos/octopus") + } + }) + + t.Run("prefix / matches everything", func(t *testing.T) { + root := serviceWithPolicy(nil, []string{"/"}) + if err := checkServicePolicy(root, requestTo(t, "GET", "/anything/at/all")); err != nil { + t.Fatalf("/ should match: %v", err) + } + }) + + t.Run("a trailing slash on the prefix is normalised away", func(t *testing.T) { + trailing := serviceWithPolicy(nil, []string{"/repos/"}) + if err := checkServicePolicy(trailing, requestTo(t, "GET", "/repos/octo")); err != nil { + t.Fatalf("/repos/ should cover /repos/octo: %v", err) + } + }) + + t.Run("an empty path reads as the root", func(t *testing.T) { + root := serviceWithPolicy(nil, []string{"/"}) + req := requestTo(t, "GET", "/") + req.URL.Path = "" + if err := checkServicePolicy(root, req); err != nil { + t.Fatalf("an empty path should read as /: %v", err) + } + }) +} + +func TestControlByteEscapesAreRefused(t *testing.T) { + svc := serviceWithPolicy(nil, []string{"/repos"}) + for _, path := range []string{"/repos/..%00/admin", "/repos/%00../admin", "/repos/x%09y", "/repos/x%7f"} { + t.Run(path, func(t *testing.T) { + if err := checkServicePolicy(svc, requestTo(t, "GET", path)); !errors.Is(err, errPolicyBlocked) { + t.Fatalf("%s should be blocked, got %v", path, err) + } + }) + } +} + +func TestWireMappingFailsClosed(t *testing.T) { + t.Run("nil stays unrestricted", func(t *testing.T) { + if toMethodSet(nil) != nil || toPathPrefixes(nil) != nil { + t.Fatal("a nil list must stay nil, which every caller reads as unrestricted") + } + }) + + t.Run("an empty restriction allows nothing", func(t *testing.T) { + methods := toMethodSet([]string{}) + if methods == nil || len(methods) != 0 { + t.Fatalf("an empty method list must restrict, got %v", methods) + } + + // "//" trims to "" the same way " " does, so both have to reach the fail-closed guard. + for _, empty := range []string{" ", "//", "///"} { + prefixes := toPathPrefixes([]string{empty}) + if len(prefixes) == 0 { + t.Fatalf("%q must restrict, not fall through to unrestricted", empty) + } + svc := &resolvedService{name: "s", allowedPathPrefixes: prefixes} + if err := checkServicePolicy(svc, requestTo(t, "GET", "/anything")); !errors.Is(err, errPolicyBlocked) { + t.Fatalf("%q: expected a block, got %v", empty, err) + } + } + }) +} + +func TestNonAsciiPathsAreJudgedByUtf8Validity(t *testing.T) { + svc := serviceWithPolicy(nil, []string{"/repos"}) + + allowed := []string{ + "/repos/owner/repo/contents/caf%C3%A9.md", + "/repos/%E6%97%A5%E6%9C%AC%E8%AA%9E", + "/repos/a%20b", + "/repos/%F0%9F%94%91", + } + for _, path := range allowed { + t.Run("allows "+path, func(t *testing.T) { + if err := checkServicePolicy(svc, requestTo(t, "GET", path)); err != nil { + t.Fatalf("%s should be allowed: %v", path, err) + } + }) + } + + // %c0%ae is an overlong '.', which some servers normalise as a traversal segment. + blocked := []string{ + "/repos/%c0%ae%c0%ae/admin", + "/repos/%c0%af", + "/repos/%e0%80%ae", + "/repos/%ff", + "/repos/%c3", + } + for _, path := range blocked { + t.Run("blocks "+path, func(t *testing.T) { + if err := checkServicePolicy(svc, requestTo(t, "GET", path)); !errors.Is(err, errPolicyBlocked) { + t.Fatalf("%s should be blocked, got %v", path, err) + } + }) + } +} + +func TestAnExplicitRootPrefixMatchesEverythingAnUnrestrictedServiceWould(t *testing.T) { + root := serviceWithPolicy(nil, []string{"/"}) + open := serviceWithPolicy(nil, nil) + + for _, path := range []string{"/anything", "/repos/caf%C3%A9.md", "/a%20b"} { + t.Run(path, func(t *testing.T) { + rootErr := checkServicePolicy(root, requestTo(t, "GET", path)) + openErr := checkServicePolicy(open, requestTo(t, "GET", path)) + if (rootErr == nil) != (openErr == nil) { + t.Fatalf("prefix / and no prefix disagree on %s: %v vs %v", path, rootErr, openErr) + } + }) + } +} + +// The post-substitution check is weaker than isAmbiguousPath on purpose, because we escape the value +// ourselves and isAmbiguousPath refuses that escaping. It still has to refuse what a value can introduce. +func TestThePostSubstitutionCheckRefusesWhatAValueCanIntroduce(t *testing.T) { + prefixes := toPathPrefixes([]string{"/repos"}) + for _, tc := range []struct { + value string + want bool + why string + }{ + {"ghp_plain", true, "an ordinary secret"}, + {"org/repo", true, "a slash is escaped by us, not a separator"}, + {"v1.2", true, "a dot inside a segment is not a dot segment"}, + {"a%b", true, "a percent is escaped by us"}, + {"", false, "an empty value leaves '//' behind"}, + {"/admin", false, "a leading slash leaves '//' behind"}, + {"admin/", false, "a trailing slash leaves '//' behind"}, + {"a//b", false, "a doubled slash inside the value"}, + {"a\tb", false, "a control byte"}, + {"\xc0\xae\xc0\xae", false, "an overlong '..'"}, + } { + req, _ := http.NewRequest("GET", "https://api.github.com/repos/__PAT__/admin", nil) + if _, err := applySubstitutions(req, "github", []substitution{subOn("__PAT__", tc.value, surfacePath)}); err != nil { + t.Fatalf("%s: %v", tc.why, err) + } + if got := pathAllowedAfterSubstitution(requestPath(req), req.URL.Path, prefixes); got != tc.want { + t.Errorf("value %q (%s): allowed = %v, want %v (path %q)", tc.value, tc.why, got, tc.want, requestPath(req)) + } + } +} + +// Go rebuilds EscapedPath from the decoded path when RawPath is not valid encoding, which drops the very +// '%2F' hasUnsafeEscape exists to refuse. Escaping those bytes first keeps the escape intact. +func TestNormalizeRequestTargetKeepsAnEscapeGoWouldDrop(t *testing.T) { + for _, tc := range []struct{ raw, want, why string }{ + {"/repos/a%2Fb", "/repos/a%2Fb", "already valid, left alone"}, + {"/repos/a%2Fb/{x}", "/repos/a%2Fb/%7Bx%7D", "the brace is escaped, the %2F survives"}, + {"/repos/{{PAT}}", "/repos/%7B%7BPAT%7D%7D", "a placeholder still reads as one"}, + {"/repos/a|b", "/repos/a%7Cb", "a pipe"}, + {"/repos/a%25b/{x}", "/repos/a%25b/%7Bx%7D", "an escaped percent is not escaped twice"}, + {"/repos/plain", "/repos/plain", "nothing to do"}, + // validEncoded accepts these, so Go never rebuilds and the guard leaves them exactly as sent. Escaping + // them would change the wire and break a byte-compared prefix. + {"/repos/a(b)!*[]'", "/repos/a(b)!*[]'", "sub-delims Go accepts unescaped"}, + {"/repos/a%2Fb/c(d)", "/repos/a%2Fb/c(d)", "an escape plus sub-delims, still valid"}, + } { + u, err := url.ParseRequestURI(tc.raw) + if err != nil { + t.Fatalf("%s: %v", tc.raw, err) + } + normalizeRequestTarget(u) + if got := u.EscapedPath(); got != tc.want { + t.Errorf("%s (%s): EscapedPath = %q, want %q", tc.raw, tc.why, got, tc.want) + } + } +} + +// The escape that used to be dropped is the one the prefix check runs on, so a single brace decided whether +// a path was refused. +func TestAnEscapedSlashIsRefusedWhateverElseThePathCarries(t *testing.T) { + prefixes := toPathPrefixes([]string{"/repos"}) + for _, raw := range []string{"/repos/a%2Fb", "/repos/a%2Fb/{x}"} { + u, err := url.ParseRequestURI(raw) + if err != nil { + t.Fatalf("%s: %v", raw, err) + } + normalizeRequestTarget(u) + if pathAllowed(u.EscapedPath(), prefixes) { + t.Errorf("%s was allowed; an escaped slash must be refused", raw) + } + } +} diff --git a/packages/agentvault/proxy.go b/packages/agentvault/proxy.go index f42049f6..5b4fd305 100644 --- a/packages/agentvault/proxy.go +++ b/packages/agentvault/proxy.go @@ -247,7 +247,8 @@ func (ps *proxyServer) handleConnect(w http.ResponseWriter, r *http.Request) { // Node's fetch tunnels http:// targets too and speaks plaintext inside, where every other client sends // absolute-form. A tunnel is opaque bytes to an ordinary proxy, so that works everywhere else; here the // 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. buffered := newBufferedConn(clientConn) _ = clientConn.SetDeadline(time.Now().Add(tlsHandshakeTimeout)) first, err := buffered.reader.Peek(1) @@ -291,8 +292,8 @@ func newBufferedConn(c net.Conn) *bufferedConn { func (b *bufferedConn) Read(p []byte) (int, error) { return b.reader.Read(p) } -// The scheme is the tunnel's, not the inner request's: a credential is only ever attached on https, and -// the target host comes from the CONNECT line so an agent cannot address one host through a tunnel to another. +// The scheme is the tunnel's, not the inner request's, and the target host comes from the CONNECT line so +// an agent cannot address one host through a tunnel to another. func (ps *proxyServer) serveTunnel(conn net.Conn, scheme, hostname, port, sessionToken string) { listener := newOneShotListener(conn) srv := &http.Server{ @@ -346,18 +347,22 @@ func (ps *proxyServer) handlePlainForward(w http.ResponseWriter, r *http.Request } func (ps *proxyServer) forwardHTTP(w http.ResponseWriter, r *http.Request, scheme, hostname, port, sessionToken string) { - // TRACE and TRACK make the upstream reflect the injected credential back in the response body. - if r.Method == http.MethodTrace || r.Method == "TRACK" { - http.Error(w, "method not allowed", http.StatusMethodNotAllowed) + // 'http:admin/secrets' parses to an empty path and a non-empty Opaque, which the upstream would receive + // as a request-target with no leading slash. handlePlainForward refuses the shape already; the tunnel + // reaches this handler directly, so the refusal belongs here where both doors meet. + if r.URL.Opaque != "" { + http.Error(w, "the request target must be a path; opaque forms are not forwarded", http.StatusBadRequest) return } - reqPath := r.URL.EscapedPath() - if len(reqPath) > maxLoggedPathLen { - reqPath = reqPath[:maxLoggedPathLen] + "...[truncated]" - } + // Before anything reads the path: the policy check, the substitutions and the forward all have to see + // the bytes the agent sent, not the ones Go rebuilds. + normalizeRequestTarget(r.URL) + + // requestPath rather than EscapedPath, so a brokered request is never recorded with a blank path. + reqPath := truncatePath(requestPath(r)) - resp, matched, err := ps.forward(r, scheme, hostname, port, sessionToken) + resp, matched, outcome, err := ps.forward(r, scheme, hostname, port, sessionToken) // The body is fixed text per outcome, never err.Error(): an APIError carries the control-plane URL and // request id, and a dial error names the upstream address. The detail goes on the log line below. @@ -366,8 +371,11 @@ func (ps *proxyServer) forwardHTTP(w http.ResponseWriter, r *http.Request, schem status := 0 body := "" switch { - case errors.Is(err, errHostBlocked): + case errors.Is(err, errHostBlocked), errors.Is(err, errPolicyBlocked): decision, status, body = decisionBlocked, http.StatusForbidden, err.Error() + case errors.Is(err, errBodyUnreadable): + // The agent's upload broke, so this is its request to retry rather than an upstream or policy failure. + decision, status, body = decisionBlocked, http.StatusBadRequest, err.Error() case isProxyTokenRejected(err): decision, status, body = decisionError, http.StatusServiceUnavailable, proxyRevokedBody case isSessionGone(err): @@ -376,8 +384,7 @@ func (ps *proxyServer) forwardHTTP(w http.ResponseWriter, r *http.Request, schem decision, status, body = decisionError, http.StatusBadGateway, "failed to resolve the session" case err != nil: decision, status, body = decisionError, http.StatusBadGateway, "failed to reach the upstream" - // brokered means a credential went out, not merely that a service matched. - case matched != nil && matched.credential.kind != credentialPassthrough: + case outcome.brokered: decision, status = decisionBrokered, resp.StatusCode default: status = resp.StatusCode @@ -400,6 +407,14 @@ func (ps *proxyServer) forwardHTTP(w http.ResponseWriter, r *http.Request, schem if matched != nil { event = event.Str("service", matched.name).Str("accessBundle", matched.accessBundleName) } + if outcome.brokered && !strings.EqualFold(scheme, "https") { + event = event.Bool("plaintext", true) + } + // The logged path is always the agent's, so without this a substitution that matched nothing reads + // exactly like one that fired. + if len(outcome.substituted) > 0 { + event = event.Strs("substituted", outcome.substituted) + } if err != nil { event = event.Err(err) } @@ -441,16 +456,36 @@ func (ps *proxyServer) blocksOffBundle(matched *resolvedService, hostname, port return matched == nil && ps.currentConfig().TrafficPolicy == TrafficPolicyBundleHosts && !ps.isAllowedHost(hostname, port) } -func (ps *proxyServer) forward(req *http.Request, scheme, hostname, port, sessionToken string) (*http.Response, *resolvedService, error) { +type forwardOutcome struct { + brokered bool + substituted []string +} + +func (ps *proxyServer) forward(req *http.Request, scheme, hostname, port, sessionToken string) (*http.Response, *resolvedService, forwardOutcome, error) { + var outcome forwardOutcome + services, err := ps.cache.get(sessionToken) if err != nil { - return nil, nil, fmt.Errorf("%w: %w", errSessionResolve, err) + return nil, nil, outcome, fmt.Errorf("%w: %w", errSessionResolve, err) + } + + // TRACE and TRACK make the upstream reflect the injected credential back in the response body. Upper + // -cased like allowsMethod already was, or a lowercase "trace" walks past. Refused here rather than in + // the handler so it is logged like every other refusal. + if method := strings.ToUpper(req.Method); method == http.MethodTrace || method == "TRACK" { + return nil, nil, outcome, fmt.Errorf("method %s echoes headers back: %w", method, errPolicyBlocked) } matched := bestMatch(services, hostname, port) if ps.blocksOffBundle(matched, hostname, port) { - return nil, nil, fmt.Errorf("no service covers host %q: %w", hostname, errHostBlocked) + return nil, nil, outcome, fmt.Errorf("no service covers host %q: %w", hostname, errHostBlocked) + } + + if matched != nil { + if err := checkServicePolicy(matched, req); err != nil { + return nil, matched, outcome, err + } } req.URL.Scheme = scheme @@ -463,24 +498,51 @@ func (ps *proxyServer) forward(req *http.Request, scheme, hostname, port, sessio // Stripped before injecting, so a client's Connection header cannot delete the credential. stripHopByHopHeaders(req.Header) + if matched != nil && matched.allowedMethods != nil { + stripMethodOverrideHeaders(req.Header) + } + if matched != nil { - // A credential is only ever injected over TLS, whatever port the pattern names. - if !strings.EqualFold(scheme, "https") { - log.Warn(). - Str("host", hostname). - Str("service", matched.name). - Msg("agent-vault: refusing to attach a credential over plaintext http") - matched = nil - } else { - 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. + substituted, err := applySubstitutions(req, matched.name, matched.substitutions) + outcome.substituted = substituted + if err != nil { + return nil, matched, outcome, err + } + outcome.brokered = injectCustomHeaders(req, matched.customHeaders) + if injectCredential(req, &matched.credential) { + outcome.brokered = true + } + if len(outcome.substituted) > 0 { + outcome.brokered = true + } + + if len(matched.allowedPathPrefixes) > 0 && containsSurface(outcome.substituted, surfacePath) { + if !pathAllowedAfterSubstitution(requestPath(req), req.URL.Path, matched.allowedPathPrefixes) { + // The path now carries the real credential, so it must not reach the body or the log. + return nil, matched, outcome, fmt.Errorf( + "service %q does not allow the path this request substitutes to: %w", + matched.name, errPolicyBlocked, + ) + } } } resp, err := ps.transport.RoundTrip(req) if err != nil { - return nil, matched, err + return nil, matched, outcome, err } - return resp, matched, nil + return resp, matched, outcome, nil +} + +func containsSurface(surfaces []string, target string) bool { + for _, surface := range surfaces { + if surface == target { + return true + } + } + return false } func (ps *proxyServer) isAllowedHost(hostname, port string) bool { diff --git a/packages/agentvault/proxy_plaintext_test.go b/packages/agentvault/proxy_plaintext_test.go new file mode 100644 index 00000000..d9ee018a --- /dev/null +++ b/packages/agentvault/proxy_plaintext_test.go @@ -0,0 +1,111 @@ +package agentvault + +import ( + "encoding/json" + "io" + "net/http" + "net/http/httptest" + "net/url" + "testing" +) + +// The leg an agent takes for an http:// upstream: absolute-form through the proxy, no CONNECT and no TLS +// anywhere. A service reaches this fixture only by naming its port, which is what opts it into plaintext. +func newPlaintextFixture(t *testing.T, build func(host string) *resolvedService) (*http.Client, string) { + t.Helper() + + upstream := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + body, _ := io.ReadAll(r.Body) + w.Header().Set("Content-Type", "application/json") + _ = json.NewEncoder(w).Encode(echoed{ + Method: r.Method, + Path: r.URL.EscapedPath(), + Query: r.URL.RawQuery, + Headers: r.Header, + Body: string(body), + }) + })) + t.Cleanup(upstream.Close) + + upstreamURL, err := url.Parse(upstream.URL) + if err != nil { + t.Fatal(err) + } + host := "127.0.0.1:" + upstreamURL.Port() + + ps := &proxyServer{transport: newUpstreamTransport()} + ps.setConfig(ProxyConfig{TrafficPolicy: TrafficPolicyAnyHost}) + ps.cache = newSessionCache(fixedResolver{services: []*resolvedService{build(host)}}, ps.pollInterval) + + front := httptest.NewServer(http.HandlerFunc(ps.dispatch)) + t.Cleanup(front.Close) + + proxyURL, _ := url.Parse(front.URL) + proxyURL.User = url.UserPassword(ProxyAuthUsername, "agv_tok") + + return &http.Client{Transport: &http.Transport{Proxy: http.ProxyURL(proxyURL)}}, host +} + +func TestEverythingAServiceCarriesIsAttachedOverPlainHTTP(t *testing.T) { + client, host := newPlaintextFixture(t, func(h string) *resolvedService { + svc := policyService(h, nil, nil, + []customHeader{{name: "X-Tenant", value: []byte("acme")}}, + []substitution{{ + placeholder: "{{PAT}}", + surfaces: map[string]bool{surfacePath: true}, + value: []byte("ghp_real"), + }}, + ) + svc.credential = credential{ + kind: credentialBearer, + headerName: "Authorization", + headerPrefix: "Bearer", + value: []byte("tok_real"), + } + return svc + }) + + status, payload := do(t, client, http.MethodGet, "http://"+host+"/repos/{{PAT}}", "") + if status != http.StatusOK { + t.Fatalf("status = %d, want 200: %s", status, payload) + } + got := decodeEcho(t, payload) + + if auth := got.Headers["Authorization"]; len(auth) != 1 || auth[0] != "Bearer tok_real" { + t.Errorf("Authorization = %v, want the real credential", auth) + } + if tenant := got.Headers["X-Tenant"]; len(tenant) != 1 || tenant[0] != "acme" { + t.Errorf("X-Tenant = %v, want the custom header", tenant) + } + if got.Path != "/repos/ghp_real" { + t.Errorf("path = %q, want the substitution applied", got.Path) + } +} + +// A pass-through service carries no credential, so before this it was refused for a credential it never +// had and its headers were dropped with it. +func TestAPassThroughServiceStillAddsItsHeadersOverPlainHTTP(t *testing.T) { + client, host := newPlaintextFixture(t, func(h string) *resolvedService { + return policyService(h, nil, nil, []customHeader{{name: "X-Tenant", value: []byte("acme")}}, nil) + }) + + status, payload := do(t, client, http.MethodGet, "http://"+host+"/things", "") + if status != http.StatusOK { + t.Fatalf("status = %d, want 200: %s", status, payload) + } + if tenant := decodeEcho(t, payload).Headers["X-Tenant"]; len(tenant) != 1 || tenant[0] != "acme" { + t.Errorf("X-Tenant = %v, want the custom header", tenant) + } +} + +// Restrictions are not loosened by the upstream being plaintext. +func TestAServiceRestrictionStillHoldsOverPlainHTTP(t *testing.T) { + client, host := newPlaintextFixture(t, func(h string) *resolvedService { + return policyService(h, []string{"GET"}, nil, nil, nil) + }) + + status, _ := do(t, client, http.MethodDelete, "http://"+host+"/things", "") + if status != http.StatusForbidden { + t.Errorf("status = %d, want 403", status) + } +} diff --git a/packages/agentvault/proxy_policy_test.go b/packages/agentvault/proxy_policy_test.go new file mode 100644 index 00000000..b78365cf --- /dev/null +++ b/packages/agentvault/proxy_policy_test.go @@ -0,0 +1,462 @@ +package agentvault + +import ( + "bytes" + "crypto/tls" + "crypto/x509" + "encoding/json" + "fmt" + "io" + "net/http" + "net/http/httptest" + "net/url" + "strings" + "testing" + + "github.com/rs/zerolog" + "github.com/rs/zerolog/log" +) + +type echoed struct { + Method string `json:"method"` + Path string `json:"path"` + Query string `json:"query"` + Headers map[string][]string `json:"headers"` + Body string `json:"body"` +} + +type fixedResolver struct{ services []*resolvedService } + +func (r fixedResolver) resolve(string) (*resolveResult, error) { + return &resolveResult{SessionID: "s1", Services: r.services}, nil +} + +// The whole path an agent's request takes: CONNECT, TLS terminated by the proxy's own CA, policy and +// injection applied, then forwarded to a real upstream that echoes what it got. The upstream is addressed as +// 127.0.0.1 so both TLS legs verify against httptest's certificate and mintLeaf's IP SAN. +func newPolicyFixture(t *testing.T, build func(host string) *resolvedService) (*http.Client, string) { + t.Helper() + + upstream := httptest.NewTLSServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + body, _ := io.ReadAll(r.Body) + w.Header().Set("Content-Type", "application/json") + _ = json.NewEncoder(w).Encode(echoed{ + Method: r.Method, + Path: r.URL.EscapedPath(), + Query: r.URL.RawQuery, + Headers: r.Header, + Body: string(body), + }) + })) + t.Cleanup(upstream.Close) + + upstreamURL, err := url.Parse(upstream.URL) + if err != nil { + t.Fatal(err) + } + host := "127.0.0.1:" + upstreamURL.Port() + + upstreamPool := x509.NewCertPool() + upstreamPool.AddCert(upstream.Certificate()) + + key, cert, err := generateRootCa() + if err != nil { + t.Fatal(err) + } + + transport := newUpstreamTransport() + transport.TLSClientConfig = &tls.Config{RootCAs: upstreamPool} + + ps := &proxyServer{transport: transport, ca: newCaManager(key, cert)} + ps.setConfig(ProxyConfig{TrafficPolicy: TrafficPolicyAnyHost}) + ps.cache = newSessionCache(fixedResolver{services: []*resolvedService{build(host)}}, ps.pollInterval) + + front := httptest.NewServer(http.HandlerFunc(ps.dispatch)) + t.Cleanup(front.Close) + + clientPool := x509.NewCertPool() + clientPool.AddCert(cert) + proxyURL, _ := url.Parse(front.URL) + proxyURL.User = url.UserPassword(ProxyAuthUsername, "agv_tok") + + client := &http.Client{ + Transport: &http.Transport{ + Proxy: http.ProxyURL(proxyURL), + TLSClientConfig: &tls.Config{RootCAs: clientPool}, + }, + } + return client, host +} + +func policyService(host string, methods, prefixes []string, customHeaders []customHeader, subs []substitution) *resolvedService { + return &resolvedService{ + name: "github", + accessBundleName: "bundle", + hostPatterns: parseHostPatterns(host), + allowedMethods: toMethodSet(methods), + allowedPathPrefixes: toPathPrefixes(prefixes), + credential: credential{kind: credentialPassthrough}, + customHeaders: customHeaders, + substitutions: subs, + } +} + +func do(t *testing.T, c *http.Client, method, target, body string) (int, string) { + t.Helper() + var reader io.Reader + if body != "" { + reader = strings.NewReader(body) + } + req, err := http.NewRequest(method, target, reader) + if err != nil { + t.Fatal(err) + } + resp, err := c.Do(req) + if err != nil { + t.Fatal(err) + } + defer resp.Body.Close() + payload, _ := io.ReadAll(resp.Body) + return resp.StatusCode, strings.TrimSpace(string(payload)) +} + +func decodeEcho(t *testing.T, payload string) echoed { + t.Helper() + var got echoed + if err := json.Unmarshal([]byte(payload), &got); err != nil { + t.Fatalf("upstream did not echo JSON (%v): %s", err, payload) + } + return got +} + +func TestMethodPolicyThroughTheTunnel(t *testing.T) { + client, host := newPolicyFixture(t, func(h string) *resolvedService { + return policyService(h, []string{"GET"}, nil, nil, nil) + }) + + status, body := do(t, client, "GET", fmt.Sprintf("https://%s/anything", host), "") + if status != http.StatusOK { + t.Fatalf("GET should reach the upstream, got %d: %s", status, body) + } + if got := decodeEcho(t, body); got.Method != "GET" { + t.Fatalf("upstream saw %q", got.Method) + } + + status, body = do(t, client, "POST", fmt.Sprintf("https://%s/anything", host), "x") + if status != http.StatusForbidden { + t.Fatalf("POST should be refused, got %d: %s", status, body) + } + if !strings.Contains(body, `service "github" does not allow POST`) { + t.Fatalf("unhelpful 403 body: %q", body) + } +} + +func TestPathPolicyThroughTheTunnel(t *testing.T) { + client, host := newPolicyFixture(t, func(h string) *resolvedService { + return policyService(h, nil, []string{"/repos"}, nil, nil) + }) + + status, body := do(t, client, "GET", fmt.Sprintf("https://%s/repos/octo/hello", host), "") + if status != http.StatusOK { + t.Fatalf("an allowed path should reach the upstream, got %d: %s", status, body) + } + + for _, path := range []string{"/repositories", "/admin", "/repos/%2e%2e/admin"} { + status, body = do(t, client, "GET", fmt.Sprintf("https://%s%s", host, path), "") + if status != http.StatusForbidden { + t.Fatalf("%s should be refused, got %d: %s", path, status, body) + } + if !strings.Contains(body, "blocked by service policy") { + t.Fatalf("%s: unhelpful 403 body: %q", path, body) + } + } +} + +func TestCustomHeadersReachTheUpstream(t *testing.T) { + client, host := newPolicyFixture(t, func(h string) *resolvedService { + return policyService(h, nil, nil, []customHeader{ + {name: "X-Org-Id", value: []byte("acme")}, + {name: "X-Api-Ver", prefix: "v", value: []byte("2")}, + }, nil) + }) + + status, body := do(t, client, "GET", fmt.Sprintf("https://%s/x", host), "") + if status != http.StatusOK { + t.Fatalf("got %d: %s", status, body) + } + got := decodeEcho(t, body) + if v := got.Headers["X-Org-Id"]; len(v) != 1 || v[0] != "acme" { + t.Fatalf("X-Org-Id = %v", v) + } + if v := got.Headers["X-Api-Ver"]; len(v) != 1 || v[0] != "v 2" { + t.Fatalf("X-Api-Ver = %v", v) + } +} + +func TestSubstitutionsReachTheUpstream(t *testing.T) { + client, host := newPolicyFixture(t, func(h string) *resolvedService { + return policyService(h, nil, nil, nil, []substitution{ + subOn("__PAT__", "real-token", surfacePath, surfaceQuery, surfaceHeader, surfaceBody), + }) + }) + + req, err := http.NewRequest( + "POST", + fmt.Sprintf("https://%s/repos/__PAT__/x?key=__PAT__", host), + strings.NewReader(`{"token":"__PAT__"}`), + ) + if err != nil { + t.Fatal(err) + } + req.Header.Set("X-Key", "Bearer __PAT__") + + resp, err := client.Do(req) + if err != nil { + t.Fatal(err) + } + defer resp.Body.Close() + payload, _ := io.ReadAll(resp.Body) + if resp.StatusCode != http.StatusOK { + t.Fatalf("got %d: %s", resp.StatusCode, payload) + } + + got := decodeEcho(t, strings.TrimSpace(string(payload))) + if got.Path != "/repos/real-token/x" { + t.Fatalf("path = %q", got.Path) + } + if got.Query != "key=real-token" { + t.Fatalf("query = %q", got.Query) + } + if v := got.Headers["X-Key"]; len(v) != 1 || v[0] != "Bearer real-token" { + t.Fatalf("X-Key = %v", v) + } + if got.Body != `{"token":"real-token"}` { + t.Fatalf("body = %q", got.Body) + } + if strings.Contains(string(payload), "__PAT__") { + t.Fatalf("a placeholder survived to the upstream: %s", payload) + } +} + +func TestABlockedSubstitutedPathNeverEchoesTheSecret(t *testing.T) { + secret := "../s3cr3tadmin" + client, host := newPolicyFixture(t, func(h string) *resolvedService { + return policyService(h, nil, []string{"/repos"}, nil, []substitution{ + subOn("__PAT__", secret, surfacePath), + }) + }) + + status, body := do(t, client, "GET", fmt.Sprintf("https://%s/repos/__PAT__", host), "") + if status != http.StatusForbidden { + t.Fatalf("expected a 403, got %d: %s", status, body) + } + if strings.Contains(body, "s3cr3t") { + t.Fatalf("the 403 body handed the injected secret back to the agent: %q", body) + } +} + +func TestAPathSubstitutionIsRecheckedAgainstThePolicy(t *testing.T) { + client, host := newPolicyFixture(t, func(h string) *resolvedService { + return policyService(h, nil, []string{"/repos"}, nil, []substitution{ + subOn("__PAT__", "../admin", surfacePath), + }) + }) + + status, body := do(t, client, "GET", fmt.Sprintf("https://%s/repos/__PAT__", host), "") + if status != http.StatusForbidden { + t.Fatalf("a substitution that escapes the prefix should be refused, got %d: %s", status, body) + } +} + +func TestASubstitutedValueCannotWalkOutOfItsPrefix(t *testing.T) { + for _, secret := range []string{`\admin`, `;x`, `/admin`} { + t.Run(secret, func(t *testing.T) { + client, host := newPolicyFixture(t, func(h string) *resolvedService { + return policyService(h, nil, []string{"/repos"}, nil, []substitution{ + subOn("__P__", secret, surfacePath), + }) + }) + + status, body := do(t, client, "GET", fmt.Sprintf("https://%s/repos/..__P__/admin", host), "") + if status != http.StatusForbidden { + t.Fatalf("expected a 403, got %d: %s", status, body) + } + }) + } +} + +func TestAPathSubstitutionMayCarryASlashUnderAPrefix(t *testing.T) { + client, host := newPolicyFixture(t, func(h string) *resolvedService { + return policyService(h, nil, []string{"/api/v4/projects"}, nil, []substitution{ + subOn("__PROJ__", "mygroup/myproject", surfacePath), + }) + }) + + status, body := do(t, client, "GET", fmt.Sprintf("https://%s/api/v4/projects/__PROJ__/pipelines", host), "") + if status != http.StatusOK { + t.Fatalf("expected a 200, got %d: %s", status, body) + } + if got := decodeEcho(t, strings.TrimSpace(body)); got.Path != "/api/v4/projects/mygroup%2Fmyproject/pipelines" { + t.Fatalf("upstream path = %q", got.Path) + } +} + +func TestACustomHeaderCannotReplaceTheCredential(t *testing.T) { + cases := []struct { + name string + cred credential + customHeaders []customHeader + wantHeader string + want string + }{ + { + name: "the default Authorization, collided case-insensitively", + cred: credential{kind: credentialBearer, headerPrefix: "Bearer", value: []byte("real-token")}, + customHeaders: []customHeader{{name: "authorization", value: []byte("spoofed")}}, + wantHeader: "Authorization", + want: "Bearer real-token", + }, + { + name: "a credential on its own header name", + cred: credential{kind: credentialBearer, headerName: "X-Org-Id", value: []byte("real-token")}, + customHeaders: []customHeader{{name: "X-Org-Id", value: []byte("spoofed")}}, + wantHeader: "X-Org-Id", + want: "real-token", + }, + { + name: "pass-through leaves the custom header alone", + cred: credential{kind: credentialPassthrough}, + customHeaders: []customHeader{{name: "Authorization", prefix: "Bearer", value: []byte("custom")}}, + wantHeader: "Authorization", + want: "Bearer custom", + }, + } + + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + client, host := newPolicyFixture(t, func(h string) *resolvedService { + svc := policyService(h, nil, nil, tc.customHeaders, nil) + svc.credential = tc.cred + return svc + }) + + status, body := do(t, client, "GET", fmt.Sprintf("https://%s/x", host), "") + if status != http.StatusOK { + t.Fatalf("got %d: %s", status, body) + } + got := decodeEcho(t, strings.TrimSpace(body)) + if v := got.Headers[tc.wantHeader]; len(v) != 1 || v[0] != tc.want { + t.Fatalf("%s = %v, want %q", tc.wantHeader, v, tc.want) + } + if strings.Contains(body, "spoofed") { + t.Fatalf("the custom header replaced the credential: %s", body) + } + }) + } +} + +func TestTheLogSaysWhichSurfacesWereSubstituted(t *testing.T) { + type line struct { + Path string `json:"path"` + Decision string `json:"decision"` + Substituted []string `json:"substituted"` + } + + capture := func(t *testing.T, target string) line { + t.Helper() + client, host := newPolicyFixture(t, func(h string) *resolvedService { + return policyService(h, nil, nil, nil, []substitution{ + subOn("__PAT__", "real-token", surfacePath, surfaceHeader), + }) + }) + + var buf bytes.Buffer + restore := log.Logger + log.Logger = zerolog.New(&buf) + defer func() { log.Logger = restore }() + + if status, body := do(t, client, "GET", fmt.Sprintf("https://%s%s", host, target), ""); status != http.StatusOK { + t.Fatalf("got %d: %s", status, body) + } + + var got line + for _, raw := range strings.Split(strings.TrimSpace(buf.String()), "\n") { + var candidate line + if json.Unmarshal([]byte(raw), &candidate) == nil && candidate.Decision != "" { + got = candidate + } + } + if got.Decision == "" { + t.Fatalf("no request line logged: %s", buf.String()) + } + return got + } + + t.Run("a substitution that fired names its surfaces", func(t *testing.T) { + got := capture(t, "/repos/__PAT__/x") + if len(got.Substituted) != 1 || got.Substituted[0] != surfacePath { + t.Fatalf("substituted = %v, want [path]", got.Substituted) + } + if got.Path != "/repos/__PAT__/x" { + t.Fatalf("path = %q", got.Path) + } + if strings.Contains(got.Path, "real-token") { + t.Fatalf("the log leaked the substituted value: %q", got.Path) + } + }) + + t.Run("a substitution that matched nothing says nothing", func(t *testing.T) { + got := capture(t, "/repos/no-placeholder-here") + if len(got.Substituted) != 0 { + t.Fatalf("substituted = %v, want empty", got.Substituted) + } + }) +} + +func TestAnEchoingMethodIsRefusedWhateverItsCase(t *testing.T) { + client, host := newPolicyFixture(t, func(h string) *resolvedService { + return policyService(h, nil, nil, nil, nil) + }) + + // Unrestricted on methods, so only the echo guard can refuse these. Go sends the method verbatim. + for _, method := range []string{"TRACE", "trace", "TRACK", "track"} { + status, body := do(t, client, method, fmt.Sprintf("https://%s/anything", host), "") + if status != http.StatusForbidden { + t.Fatalf("%s should be refused, got %d: %s", method, status, body) + } + if !strings.Contains(body, "echoes headers back") { + t.Fatalf("%s: unhelpful body %q", method, body) + } + } +} + +func TestAMethodOverrideHeaderCannotOutrankTheAllowlist(t *testing.T) { + client, host := newPolicyFixture(t, func(h string) *resolvedService { + return policyService(h, []string{"GET", "POST"}, nil, nil, nil) + }) + + req, err := http.NewRequest("POST", fmt.Sprintf("https://%s/anything", host), strings.NewReader("x")) + if err != nil { + t.Fatal(err) + } + req.Header.Set("X-HTTP-Method-Override", "DELETE") + req.Header.Set("X-Method-Override", "DELETE") + req.Header.Set("X-HTTP-Method", "DELETE") + + resp, err := client.Do(req) + if err != nil { + t.Fatal(err) + } + defer resp.Body.Close() + raw, _ := io.ReadAll(resp.Body) + if resp.StatusCode != http.StatusOK { + t.Fatalf("the POST itself is allowed, got %d: %s", resp.StatusCode, raw) + } + + got := decodeEcho(t, string(raw)) + for _, name := range []string{"X-Http-Method-Override", "X-Method-Override", "X-Http-Method"} { + if v := got.Headers[name]; len(v) > 0 { + t.Fatalf("%s reached the upstream as %v, so the allowlist can be outranked", name, v) + } + } +} diff --git a/packages/agentvault/proxy_requesttarget_test.go b/packages/agentvault/proxy_requesttarget_test.go new file mode 100644 index 00000000..e7c6de1b --- /dev/null +++ b/packages/agentvault/proxy_requesttarget_test.go @@ -0,0 +1,197 @@ +package agentvault + +import ( + "bufio" + "crypto/tls" + "crypto/x509" + "encoding/json" + "fmt" + "io" + "net" + "net/http" + "net/http/httptest" + "net/url" + "testing" +) + +// Literal bytes on the wire, because a Go client escapes a brace before sending and the whole point is what +// an agent that does not can make the proxy do. +func rawProxyRequest(t *testing.T, proxyHost, requestLine, hostHeader, tunnelTo string) (*http.Response, string) { + t.Helper() + conn, err := net.Dial("tcp", proxyHost) + if err != nil { + t.Fatal(err) + } + t.Cleanup(func() { _ = conn.Close() }) + reader := bufio.NewReader(conn) + + if tunnelTo != "" { + fmt.Fprintf(conn, "CONNECT %s HTTP/1.1\r\nHost: %s\r\nProxy-Authorization: Basic %s\r\n\r\n", + tunnelTo, tunnelTo, testProxyAuth) + established, err := http.ReadResponse(reader, nil) + if err != nil { + t.Fatal(err) + } + if established.StatusCode != http.StatusOK { + t.Fatalf("CONNECT = %d", established.StatusCode) + } + } + + fmt.Fprintf(conn, "%s\r\nHost: %s\r\nProxy-Authorization: Basic %s\r\nConnection: close\r\n\r\n", + requestLine, hostHeader, testProxyAuth) + resp, err := http.ReadResponse(reader, nil) + if err != nil { + t.Fatal(err) + } + payload, _ := io.ReadAll(resp.Body) + _ = resp.Body.Close() + return resp, string(payload) +} + +// base64("x-agent-vault:agv_tok") +const testProxyAuth = "eC1hZ2VudC12YXVsdDphZ3ZfdG9r" + +func newRequestTargetFixture(t *testing.T, prefixes []string) (proxyHost, upstreamHost string) { + t.Helper() + upstream := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + _ = json.NewEncoder(w).Encode(echoed{Path: r.URL.EscapedPath()}) + })) + t.Cleanup(upstream.Close) + uu, _ := url.Parse(upstream.URL) + upstreamHost = "127.0.0.1:" + uu.Port() + + key, cert, err := generateRootCa() + if err != nil { + t.Fatal(err) + } + ps := &proxyServer{transport: newUpstreamTransport(), ca: newCaManager(key, cert)} + ps.setConfig(ProxyConfig{TrafficPolicy: TrafficPolicyAnyHost}) + ps.cache = newSessionCache(fixedResolver{services: []*resolvedService{ + policyService(upstreamHost, nil, prefixes, nil, []substitution{ + {placeholder: "{{PAT}}", surfaces: map[string]bool{surfacePath: true}, value: []byte("ghp_real")}, + }), + }}, ps.pollInterval) + + front := httptest.NewServer(http.HandlerFunc(ps.dispatch)) + t.Cleanup(front.Close) + fu, _ := url.Parse(front.URL) + return fu.Host, upstreamHost +} + +// One literal brace used to make Go rebuild the path, dropping the '%2F' the prefix check refuses, and the +// upstream received two segments where the agent sent one. +func TestALiteralBraceNoLongerHidesAnEscapedSlash(t *testing.T) { + proxyHost, upstreamHost := newRequestTargetFixture(t, []string{"/repos"}) + + resp, _ := rawProxyRequest(t, proxyHost, + fmt.Sprintf("GET http://%s/repos/a%%2Fb/{x} HTTP/1.1", upstreamHost), upstreamHost, "") + if resp.StatusCode != http.StatusForbidden { + t.Errorf("status = %d, want 403", resp.StatusCode) + } +} + +func TestAPlaceholderInThePathStillSubstitutes(t *testing.T) { + proxyHost, upstreamHost := newRequestTargetFixture(t, []string{"/repos"}) + + resp, payload := rawProxyRequest(t, proxyHost, + fmt.Sprintf("GET http://%s/repos/{{PAT}} HTTP/1.1", upstreamHost), upstreamHost, "") + if resp.StatusCode != http.StatusOK { + t.Fatalf("status = %d, want 200: %s", resp.StatusCode, payload) + } + var got echoed + if err := json.Unmarshal([]byte(payload), &got); err != nil { + t.Fatalf("upstream did not echo JSON (%v): %s", err, payload) + } + if got.Path != "/repos/ghp_real" { + t.Errorf("upstream saw %q, want the substituted path", got.Path) + } +} + +// 'http:admin/secrets' parses to an empty path and a non-empty Opaque. handlePlainForward refuses the shape, +// the tunnel reaches forwardHTTP directly, and the upstream would have received a target with no leading +// slash and a real credential on it. +func TestAnOpaqueRequestTargetIsRefusedInsideATunnel(t *testing.T) { + proxyHost, upstreamHost := newRequestTargetFixture(t, nil) + + resp, _ := rawProxyRequest(t, proxyHost, "GET http:admin/secrets HTTP/1.1", upstreamHost, upstreamHost) + if resp.StatusCode != http.StatusBadRequest { + t.Errorf("status = %d, want 400", resp.StatusCode) + } +} + +// The same two cases through a real CONNECT + TLS tunnel, which is how an agent actually arrives. The raw +// dial is still required: a Go client escapes the brace before sending, which is the bug's blind spot. +func TestTheRequestTargetHoldsThroughATLSTunnel(t *testing.T) { + upstream := httptest.NewTLSServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + _ = json.NewEncoder(w).Encode(echoed{Path: r.URL.EscapedPath()}) + })) + defer upstream.Close() + uu, _ := url.Parse(upstream.URL) + upstreamHost := "127.0.0.1:" + uu.Port() + + pool := x509.NewCertPool() + pool.AddCert(upstream.Certificate()) + transport := newUpstreamTransport() + transport.TLSClientConfig = &tls.Config{RootCAs: pool} + + key, cert, err := generateRootCa() + if err != nil { + t.Fatal(err) + } + ps := &proxyServer{transport: transport, ca: newCaManager(key, cert)} + ps.setConfig(ProxyConfig{TrafficPolicy: TrafficPolicyAnyHost}) + ps.cache = newSessionCache(fixedResolver{services: []*resolvedService{ + policyService(upstreamHost, nil, []string{"/repos"}, nil, []substitution{ + {placeholder: "{{PAT}}", surfaces: map[string]bool{surfacePath: true}, value: []byte("ghp_real")}, + }), + }}, ps.pollInterval) + front := httptest.NewServer(http.HandlerFunc(ps.dispatch)) + defer front.Close() + fu, _ := url.Parse(front.URL) + + clientPool := x509.NewCertPool() + clientPool.AddCert(cert) + + send := func(target string) (int, string) { + conn, err := net.Dial("tcp", fu.Host) + if err != nil { + t.Fatal(err) + } + defer conn.Close() + fmt.Fprintf(conn, "CONNECT %s HTTP/1.1\r\nHost: %s\r\nProxy-Authorization: Basic %s\r\n\r\n", + upstreamHost, upstreamHost, testProxyAuth) + reader := bufio.NewReader(conn) + if established, err := http.ReadResponse(reader, nil); err != nil || established.StatusCode != http.StatusOK { + t.Fatalf("CONNECT failed: %v", err) + } + + tlsConn := tls.Client(conn, &tls.Config{RootCAs: clientPool, ServerName: "127.0.0.1"}) + if err := tlsConn.Handshake(); err != nil { + t.Fatal(err) + } + fmt.Fprintf(tlsConn, "GET %s HTTP/1.1\r\nHost: %s\r\nConnection: close\r\n\r\n", target, upstreamHost) + resp, err := http.ReadResponse(bufio.NewReader(tlsConn), nil) + if err != nil { + t.Fatal(err) + } + payload, _ := io.ReadAll(resp.Body) + _ = resp.Body.Close() + return resp.StatusCode, string(payload) + } + + if status, _ := send("/repos/a%2Fb/{x}"); status != http.StatusForbidden { + t.Errorf("an escaped slash beside a brace = %d, want 403", status) + } + + status, payload := send("/repos/{{PAT}}") + if status != http.StatusOK { + t.Fatalf("placeholder = %d, want 200: %s", status, payload) + } + var got echoed + if err := json.Unmarshal([]byte(payload), &got); err != nil { + t.Fatalf("upstream did not echo JSON (%v): %s", err, payload) + } + if got.Path != "/repos/ghp_real" { + t.Errorf("upstream saw %q, want the substituted path", got.Path) + } +} diff --git a/packages/agentvault/resolve.go b/packages/agentvault/resolve.go index 79e8f0ac..25e0d503 100644 --- a/packages/agentvault/resolve.go +++ b/packages/agentvault/resolve.go @@ -1,6 +1,8 @@ package agentvault import ( + "sort" + "strings" "time" "github.com/Infisical/infisical-merge/packages/api" @@ -53,11 +55,15 @@ func (r *infisicalResolver) resolve(sessionToken string) (*resolveResult, error) services := make([]*resolvedService, 0, len(res.Services)) for _, wire := range res.Services { services = append(services, &resolvedService{ - id: wire.ID, - name: wire.Name, - accessBundleName: wire.AccessBundleName, - hostPatterns: parseHostPatterns(wire.HostPattern), - credential: toCredential(wire.Credential), + id: wire.ID, + name: wire.Name, + accessBundleName: wire.AccessBundleName, + hostPatterns: parseHostPatterns(wire.HostPattern), + allowedMethods: toMethodSet(wire.AllowedMethods), + allowedPathPrefixes: toPathPrefixes(wire.AllowedPathPrefixes), + credential: toCredential(wire.Credential), + customHeaders: toCustomHeaders(wire.CustomHeaders), + substitutions: toSubstitutions(wire.Substitutions), }) } @@ -83,3 +89,74 @@ func toCredential(wire api.AgentVaultCredential) credential { return credential{kind: credentialPassthrough} } } + +func toMethodSet(methods []string) map[string]bool { + if methods == nil { + return nil + } + set := make(map[string]bool, len(methods)) + for _, method := range methods { + set[strings.ToUpper(strings.TrimSpace(method))] = true + } + return set +} + +// Normalised the same way the backend stores them, so a trailing slash cannot make a prefix unmatchable. +// +// Fails closed like toMethodSet: nil means unrestricted, and anything else means restricted, including a +// list the server sent with nothing usable in it. Dropping to a zero-length slice there would read as +// unrestricted at every call site, which is the opposite of what a restriction that arrived empty means. +func toPathPrefixes(prefixes []string) []string { + if prefixes == nil { + return nil + } + out := make([]string, 0, len(prefixes)) + for _, prefix := range prefixes { + prefix = strings.TrimSpace(prefix) + if prefix != "/" { + prefix = strings.TrimRight(prefix, "/") + } + // After the trim, so an all-slashes prefix cannot arrive here as "" and match every path. + if prefix == "" { + continue + } + out = append(out, prefix) + } + if len(out) == 0 { + // A prefix no request can match, so a restriction the server sent empty allows nothing. + return []string{"\x00"} + } + return out +} + +func toCustomHeaders(wire []api.AgentVaultCustomHeader) []customHeader { + if len(wire) == 0 { + return nil + } + customHeaders := make([]customHeader, 0, len(wire)) + for _, h := range wire { + customHeaders = append(customHeaders, customHeader{name: h.Name, prefix: h.Prefix, value: []byte(h.Value)}) + } + return customHeaders +} + +func toSubstitutions(wire []api.AgentVaultSubstitution) []substitution { + if len(wire) == 0 { + return nil + } + subs := make([]substitution, 0, len(wire)) + for _, s := range wire { + surfaces := make(map[string]bool, len(s.Surfaces)) + for _, surface := range s.Surfaces { + surfaces[surface] = true + } + subs = append(subs, substitution{placeholder: s.Placeholder, surfaces: surfaces, value: []byte(s.Value)}) + } + // Longest first, so a placeholder that starts with another one is swapped before the shorter one can + // eat its prefix and leave the tail behind. Sorted here rather than per request: the slice is shared by + // every request the session serves. + sort.SliceStable(subs, func(i, j int) bool { + return len(subs[i].placeholder) > len(subs[j].placeholder) + }) + return subs +} diff --git a/packages/agentvault/rewrite.go b/packages/agentvault/rewrite.go index 009fb088..811992c2 100644 --- a/packages/agentvault/rewrite.go +++ b/packages/agentvault/rewrite.go @@ -1,18 +1,30 @@ package agentvault import ( + "bytes" "encoding/base64" + "fmt" + "io" "net/http" + "net/url" "strings" + + "github.com/rs/zerolog/log" ) const ( credentialBearer = "bearer" credentialBasic = "basic" credentialPassthrough = "passthrough" + + surfacePath = "path" + surfaceQuery = "query" + surfaceHeader = "header" + surfaceBody = "body" + + maxBodyRewriteSize = 10 * 1024 * 1024 ) -// injectCredential overwrites an existing header on the agent's request, silently and deliberately. func injectCredential(req *http.Request, cred *credential) bool { switch cred.kind { case credentialBearer: @@ -35,12 +47,270 @@ func injectCredential(req *http.Request, cred *credential) bool { } } +// Written before the credential, so one colliding with the credential's header loses to it. Pass-through +// injects nothing, which is why Authorization as a custom header on one still works. +func injectCustomHeaders(req *http.Request, customHeaders []customHeader) bool { + for _, header := range customHeaders { + value := string(header.value) + if header.prefix != "" { + value = header.prefix + " " + value + } + req.Header.Set(header.name, value) + } + return len(customHeaders) > 0 +} + +// A body it cannot rewrite is logged rather than skipped in silence: the placeholder goes upstream and the +// agent would otherwise see only a third-party 401. +func applySubstitutions(req *http.Request, serviceName string, subs []substitution) ([]string, error) { + changed := map[string]bool{} + for _, sub := range subs { + if len(sub.placeholder) == 0 { + continue + } + real := string(sub.value) + before := len(changed) + + // Swapped in the escaped path so every other segment keeps the byte form the agent sent. Rewriting the + // decoded Path makes Go re-derive the wire path without re-escaping '/', and `group%2Fproject` would + // arrive as two segments pointing at a different resource. + if sub.surfaces[surfacePath] { + escaped := req.URL.EscapedPath() + // normalizeRequestTarget has already escaped whatever Go would have objected to, so a `{{TOKEN}}` + // on the wire reads here as `%7B%7BTOKEN%7D%7D` and matching only the typed form would miss it. + needle := sub.placeholder + if !strings.Contains(escaped, needle) { + // Percent-escapes carry no required case and clients differ, so the fallback matches against + // upper-cased escapes. Rewriting them is free here: substituting changes the URL anyway, so a + // request the agent signed itself could never have used this surface. + needle = escapedPathForm(sub.placeholder) + escaped = upperPercentEscapes(escaped) + } + if strings.Contains(escaped, needle) { + if v, ok := replaceWithinLimit(escaped, needle, url.PathEscape(real), maxBodyRewriteSize); ok { + if decoded, err := url.PathUnescape(v); err == nil { + // Go uses RawPath only when it agrees with Path, so both are written. + req.URL.Path = decoded + req.URL.RawPath = v + changed[surfacePath] = true + } + } + } + } + + // Escaped, because RawQuery goes on the wire verbatim. A base64 key containing '+' would otherwise + // arrive as a space, and one containing '&' would split into a second parameter. + if sub.surfaces[surfaceQuery] { + // A client that builds the query from parameters rather than a string percent-encodes the + // placeholder first, so `{{TOKEN}}` arrives as `%7B%7BTOKEN%7D%7D`. The path surface already + // falls back this way; without it the placeholder reaches the third party and the 401 that + // comes back says nothing about why. + rawQuery := req.URL.RawQuery + needle := sub.placeholder + if !strings.Contains(rawQuery, needle) { + needle = queryEscapedForm(sub.placeholder) + rawQuery = upperPercentEscapes(rawQuery) + } + if strings.Contains(rawQuery, needle) { + if v, ok := replaceWithinLimit(rawQuery, needle, queryValueEscape(real), maxBodyRewriteSize); ok { + req.URL.RawQuery = v + changed[surfaceQuery] = true + } + } + } + + if sub.surfaces[surfaceHeader] { + for name, values := range req.Header { + for i, v := range values { + if !strings.Contains(v, sub.placeholder) { + continue + } + if replaced, ok := replaceWithinLimit(v, sub.placeholder, real, maxBodyRewriteSize); ok { + req.Header[name][i] = replaced + changed[surfaceHeader] = true + } + } + } + } + + // The body is rewritten after this loop, so a substitution that reaches it is judged there. Anything + // else that matched nothing sent its placeholder upstream, and the third party's 401 says nothing + // about why. + if !sub.surfaces[surfaceBody] && len(changed) == before { + log.Warn(). + Str("service", serviceName). + Str("placeholder", sub.placeholder). + Msg("agent-vault: a substitution matched nothing in the request") + } + } + + // The path, query and header surfaces are already rewritten by now, so a body that cannot be read has to + // hand back what fired alongside the error. The request is refused, but the record still has to say the + // credential was written into it. + var bodyErr error + if bodySubstitutions(subs) && req.Body != nil { + replaced, err := applyBodySubstitutions(req, serviceName, subs) + if replaced { + changed[surfaceBody] = true + } + bodyErr = err + } + + surfaces := make([]string, 0, len(changed)) + for _, surface := range []string{surfacePath, surfaceQuery, surfaceHeader, surfaceBody} { + if changed[surface] { + surfaces = append(surfaces, surface) + } + } + return surfaces, bodyErr +} + +// The placeholder as EscapedPath would render it. The leading '/' keeps url.URL's `Path == "*"` case out of +// it, and the encoder leaves a slash alone, so trimming it back off is exact. +// QueryEscape is form encoding, where a space becomes '+'. Anything reading the raw query per RFC 3986, +// SigV4 signing among them, takes that as a literal plus. Every other special is already %XX by then, so +// the only '+' left to rewrite is a space. +func queryValueEscape(value string) string { + return strings.ReplaceAll(url.QueryEscape(value), "+", "%20") +} + +func queryEscapedForm(placeholder string) string { + return queryValueEscape(placeholder) +} + +// Percent-escapes are case-insensitive, so matching is done against a copy with the hex digits upper-cased. +// Same length as the input, so nothing else about the string moves. +func upperPercentEscapes(s string) string { + if !strings.Contains(s, "%") { + return s + } + b := []byte(s) + for i := 0; i+2 < len(b); i++ { + if b[i] != '%' { + continue + } + b[i+1] = upperHexDigit(b[i+1]) + b[i+2] = upperHexDigit(b[i+2]) + } + return string(b) +} + +func upperHexDigit(c byte) byte { + if c >= 'a' && c <= 'f' { + return c - 'a' + 'A' + } + return c +} + +func escapedPathForm(placeholder string) string { + return strings.TrimPrefix((&url.URL{Path: "/" + placeholder}).EscapedPath(), "/") +} + +func bodySubstitutions(subs []substitution) bool { + for _, sub := range subs { + if sub.surfaces[surfaceBody] { + return true + } + } + return false +} + +func applyBodySubstitutions(req *http.Request, serviceName string, subs []substitution) (bool, error) { + if req.Body == http.NoBody || req.ContentLength == 0 { + return false, nil + } + if req.Header.Get("Content-Encoding") != "" { + log.Warn(). + Str("service", serviceName). + Bool("hasContentEncoding", true). + Msg("agent-vault: body substitution skipped on an encoded body; the placeholder is going upstream unchanged") + return false, nil + } + // Judged before reading, so an oversize body costs no memory. The check below still has to stand on its + // own: a chunked request declares -1, and a declared length is a claim rather than a fact. + if req.ContentLength > maxBodyRewriteSize { + log.Warn().Str("service", serviceName).Int("limitBytes", maxBodyRewriteSize). + Int64("declaredBytes", req.ContentLength). + Msg("agent-vault: body larger than the substitution limit; the placeholder is going upstream unchanged") + return false, nil + } + + body, err := io.ReadAll(io.LimitReader(req.Body, maxBodyRewriteSize+1)) + if err != nil { + // Refused outright rather than forwarded short. This used to leave ContentLength disagreeing with the + // bytes so http.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 with the credential on it and could not + // tell. A partial write is not something an agent can take back. + _ = req.Body.Close() + log.Warn().Err(err).Str("service", serviceName).Int("bytesRead", len(body)). + Msg("agent-vault: could not read the whole request body for substitution; refusing to forward a truncated one") + return false, errBodyUnreadable + } + if len(body) > maxBodyRewriteSize { + req.Body = io.NopCloser(io.MultiReader(bytes.NewReader(body), req.Body)) + log.Warn().Str("service", serviceName).Int("limitBytes", maxBodyRewriteSize). + Msg("agent-vault: body larger than the substitution limit; the placeholder is going upstream unchanged") + return false, nil + } + _ = req.Body.Close() + + rewritten := body + replaced := false + for _, sub := range subs { + if !sub.surfaces[surfaceBody] || len(sub.placeholder) == 0 { + continue + } + count := bytes.Count(rewritten, []byte(sub.placeholder)) + if count == 0 { + continue + } + if len(rewritten)+count*(len(sub.value)-len(sub.placeholder)) > maxBodyRewriteSize { + log.Warn().Str("service", serviceName).Int("limitBytes", maxBodyRewriteSize). + Msg("agent-vault: substituted body would exceed the limit; the placeholder is going upstream unchanged") + continue + } + rewritten = bytes.ReplaceAll(rewritten, []byte(sub.placeholder), sub.value) + replaced = true + } + + if len(rewritten) == 0 { + // A NopCloser over an empty reader reads to net/http as "length unknown", which turns a bodyless + // POST into a chunked request. Signing schemes and some gateways reject that. + req.Body = http.NoBody + } else { + req.Body = io.NopCloser(bytes.NewReader(rewritten)) + } + req.ContentLength = int64(len(rewritten)) + req.Header.Set("Content-Length", fmt.Sprintf("%d", len(rewritten))) + return replaced, nil +} + +// Returns the input unchanged when the expansion would exceed limit, so a short placeholder mapped to a long +// secret cannot balloon proxy memory. +func replaceWithinLimit(s, old, replacement string, limit int) (string, bool) { + count := strings.Count(s, old) + if count == 0 { + return s, true + } + delta := len(replacement) - len(old) + if delta > 0 { + // Division rather than count*delta, which overflows int on the 386 and armv6 builds and wraps to a + // negative, answering "small enough" for something that then fails to allocate. + room := limit - len(s) + if room < 0 || count > room/delta { + return s, false + } + } else if len(s)+count*delta > limit { + return s, false + } + return strings.ReplaceAll(s, old, replacement), true +} + // stripHopByHopHeaders also deletes Upgrade, which is why WebSocket upgrades cannot be forwarded. // Callers strip before injecting a credential: a Connection list naming the credential's header would // otherwise delete it, which is the same trap Go documents on httputil.ReverseProxy.Director. func stripHopByHopHeaders(header http.Header) { - // Connection names the headers meant for this hop alone, so read it before deleting it. Deleting it - // first would forward the marked header with nothing left to say it was hop-by-hop. + // Read Connection before deleting it, or the headers it names go on with nothing marking them. for _, values := range header.Values("Connection") { for _, name := range strings.Split(values, ",") { if name = strings.TrimSpace(name); name != "" { diff --git a/packages/agentvault/rewrite_transformations_test.go b/packages/agentvault/rewrite_transformations_test.go new file mode 100644 index 00000000..7077d8d9 --- /dev/null +++ b/packages/agentvault/rewrite_transformations_test.go @@ -0,0 +1,445 @@ +package agentvault + +import ( + "bytes" + "errors" + "fmt" + "io" + "net/http" + "net/url" + "strings" + "testing" + + "github.com/Infisical/infisical-merge/packages/api" +) + +func subOn(placeholder, value string, surfaces ...string) substitution { + set := map[string]bool{} + for _, surface := range surfaces { + set[surface] = true + } + return substitution{placeholder: placeholder, surfaces: set, value: []byte(value)} +} + +func TestInjectCustomHeaders(t *testing.T) { + t.Run("writes name, prefix and value", func(t *testing.T) { + req, _ := http.NewRequest("GET", "https://api.github.com/x", nil) + injectCustomHeaders(req, []customHeader{ + {name: "X-Org-Id", prefix: "", value: []byte("acme")}, + {name: "X-Api-Ver", prefix: "v", value: []byte("2")}, + }) + if got := req.Header.Get("X-Org-Id"); got != "acme" { + t.Fatalf("X-Org-Id = %q", got) + } + if got := req.Header.Get("X-Api-Ver"); got != "v 2" { + t.Fatalf("X-Api-Ver = %q", got) + } + }) + + t.Run("overwrites whatever the agent sent", func(t *testing.T) { + req, _ := http.NewRequest("GET", "https://api.github.com/x", nil) + req.Header.Set("X-Org-Id", "spoofed") + injectCustomHeaders(req, []customHeader{{name: "X-Org-Id", value: []byte("acme")}}) + if got := req.Header.Get("X-Org-Id"); got != "acme" { + t.Fatalf("X-Org-Id = %q", got) + } + }) + + t.Run("a Connection header cannot delete an injected custom header", func(t *testing.T) { + req, _ := http.NewRequest("GET", "https://api.github.com/x", nil) + req.Header.Set("Connection", "X-Org-Id") + stripHopByHopHeaders(req.Header) + injectCustomHeaders(req, []customHeader{{name: "X-Org-Id", value: []byte("acme")}}) + if got := req.Header.Get("X-Org-Id"); got != "acme" { + t.Fatalf("X-Org-Id = %q", got) + } + }) +} + +type unreadableBody struct{ t *testing.T } + +func (b *unreadableBody) Read([]byte) (int, error) { + b.t.Fatal("body was read even though the declared length is over the limit") + return 0, nil +} + +func (b *unreadableBody) Close() error { return nil } + +func TestAnOversizedDeclaredBodyIsNeverRead(t *testing.T) { + req, _ := http.NewRequest("POST", "https://api.github.com/x", nil) + req.Body = &unreadableBody{t: t} + req.ContentLength = maxBodyRewriteSize + 1 + + replaced, _ := applyBodySubstitutions(req, "github", []substitution{subOn("__PAT__", "real", surfaceBody)}) + if replaced { + t.Fatal("reported a substitution on a body it should not have touched") + } +} + +type halfBody struct { + data []byte + n int + limit int +} + +func (b *halfBody) Read(p []byte) (int, error) { + if b.n >= b.limit { + return 0, errors.New("unexpected EOF") + } + c := copy(p, b.data[b.n:b.limit]) + b.n += c + return c, nil +} + +func (b *halfBody) Close() error { return nil } + +func TestABrokenUploadIsNotForwardedTruncated(t *testing.T) { + full := strings.Repeat("A", 500) + "__PAT__" + strings.Repeat("B", 500) + req, _ := http.NewRequest("POST", "https://api.github.com/x", nil) + req.Body = &halfBody{data: []byte(full), limit: 300} + req.ContentLength = int64(len(full)) + req.Header.Set("Content-Length", fmt.Sprintf("%d", len(full))) + + _, err := applyBodySubstitutions(req, "github", []substitution{subOn("__PAT__", "real", surfaceBody)}) + + if !errors.Is(err, errBodyUnreadable) { + t.Fatalf("err = %v, want errBodyUnreadable; a partial body must not reach the upstream", err) + } + if req.ContentLength != int64(len(full)) { + t.Fatalf("ContentLength = %d, want the declared %d left alone", req.ContentLength, len(full)) + } + if got := req.Header.Get("Content-Length"); got != fmt.Sprintf("%d", len(full)) { + t.Fatalf("Content-Length header = %q, want the declared length", got) + } + sent, _ := io.ReadAll(req.Body) + if len(sent) >= len(full) { + t.Fatalf("body = %d bytes, expected only the part that was read", len(sent)) + } +} + +func TestApplySubstitutions(t *testing.T) { + t.Run("path", func(t *testing.T) { + req, _ := http.NewRequest("GET", "https://api.github.com/repos/__TOKEN__/x", nil) + surfaces, _ := applySubstitutions(req, "github", []substitution{subOn("__TOKEN__", "real", surfacePath)}) + if req.URL.Path != "/repos/real/x" { + t.Fatalf("path = %q", req.URL.Path) + } + if len(surfaces) != 1 || surfaces[0] != surfacePath { + t.Fatalf("surfaces = %v", surfaces) + } + }) + + t.Run("path, placeholder Go re-encodes", func(t *testing.T) { + req, _ := http.NewRequest("GET", "https://gitlab.com/api/v4/projects/{{PROJECT}}/pipelines", nil) + surfaces, _ := applySubstitutions(req, "gitlab", []substitution{subOn("{{PROJECT}}", "group/project", surfacePath)}) + if len(surfaces) != 1 || surfaces[0] != surfacePath { + t.Fatalf("surfaces = %v", surfaces) + } + if got := req.URL.RequestURI(); got != "/api/v4/projects/group%2Fproject/pipelines" { + t.Fatalf("wire path = %q", got) + } + }) + + t.Run("query", func(t *testing.T) { + req, _ := http.NewRequest("GET", "https://api.github.com/x?key=__TOKEN__", nil) + applySubstitutions(req, "github", []substitution{subOn("__TOKEN__", "real", surfaceQuery)}) + if req.URL.RawQuery != "key=real" { + t.Fatalf("query = %q", req.URL.RawQuery) + } + }) + + t.Run("header", func(t *testing.T) { + req, _ := http.NewRequest("GET", "https://api.github.com/x", nil) + req.Header.Set("X-Key", "Bearer __TOKEN__") + applySubstitutions(req, "github", []substitution{subOn("__TOKEN__", "real", surfaceHeader)}) + if got := req.Header.Get("X-Key"); got != "Bearer real" { + t.Fatalf("X-Key = %q", got) + } + }) + + t.Run("body, with Content-Length corrected", func(t *testing.T) { + body := `{"token":"__TOKEN__"}` + req, _ := http.NewRequest("POST", "https://api.github.com/x", strings.NewReader(body)) + applySubstitutions(req, "github", []substitution{subOn("__TOKEN__", "realvalue", surfaceBody)}) + got, _ := io.ReadAll(req.Body) + want := `{"token":"realvalue"}` + if string(got) != want { + t.Fatalf("body = %q", got) + } + if req.ContentLength != int64(len(want)) { + t.Fatalf("ContentLength = %d, want %d", req.ContentLength, len(want)) + } + }) + + t.Run("a surface the substitution does not name is left alone", func(t *testing.T) { + req, _ := http.NewRequest("GET", "https://api.github.com/repos/__TOKEN__", nil) + req.Header.Set("X-Key", "__TOKEN__") + applySubstitutions(req, "github", []substitution{subOn("__TOKEN__", "real", surfaceHeader)}) + if req.URL.Path != "/repos/__TOKEN__" { + t.Fatalf("path should be untouched, got %q", req.URL.Path) + } + if got := req.Header.Get("X-Key"); got != "real" { + t.Fatalf("X-Key = %q", got) + } + }) + + t.Run("an encoded body is forwarded untouched", func(t *testing.T) { + body := `{"token":"__TOKEN__"}` + req, _ := http.NewRequest("POST", "https://api.github.com/x", strings.NewReader(body)) + req.Header.Set("Content-Encoding", "gzip") + surfaces, _ := applySubstitutions(req, "github", []substitution{subOn("__TOKEN__", "real", surfaceBody)}) + got, _ := io.ReadAll(req.Body) + if string(got) != body { + t.Fatalf("body should be untouched, got %q", got) + } + if len(surfaces) != 0 { + t.Fatalf("nothing should be reported as changed, got %v", surfaces) + } + }) + + t.Run("a body over the limit is forwarded untouched and still readable", func(t *testing.T) { + body := strings.Repeat("a", maxBodyRewriteSize+10) + "__TOKEN__" + req, _ := http.NewRequest("POST", "https://api.github.com/x", strings.NewReader(body)) + applySubstitutions(req, "github", []substitution{subOn("__TOKEN__", "real", surfaceBody)}) + got, _ := io.ReadAll(req.Body) + if !bytes.Equal(got, []byte(body)) { + t.Fatalf("an oversize body must be forwarded byte for byte (got %d bytes, want %d)", len(got), len(body)) + } + }) + + t.Run("a body with no placeholder in it is untouched", func(t *testing.T) { + body := `{"a":"b"}` + req, _ := http.NewRequest("POST", "https://api.github.com/x", strings.NewReader(body)) + surfaces, _ := applySubstitutions(req, "github", []substitution{subOn("__TOKEN__", "real", surfaceBody)}) + got, _ := io.ReadAll(req.Body) + if string(got) != body { + t.Fatalf("body = %q", got) + } + if len(surfaces) != 0 { + t.Fatalf("surfaces = %v", surfaces) + } + }) + + t.Run("several substitutions apply to one request", func(t *testing.T) { + req, _ := http.NewRequest("GET", "https://api.github.com/repos/__A__", nil) + req.Header.Set("X-Key", "__B__") + applySubstitutions(req, "github", []substitution{ + subOn("__A__", "one", surfacePath), + subOn("__B__", "two", surfaceHeader), + }) + if req.URL.Path != "/repos/one" { + t.Fatalf("path = %q", req.URL.Path) + } + if got := req.Header.Get("X-Key"); got != "two" { + t.Fatalf("X-Key = %q", got) + } + }) +} + +// A placeholder that starts with another one is only swapped correctly when the longer runs first, and the +// server is free to send them in either order. +func TestAPlaceholderPrefixingAnotherStillSendsItsOwnSecret(t *testing.T) { + short := api.AgentVaultSubstitution{Placeholder: "__TOKEN__", Surfaces: []string{"header"}, Value: "SECRET_A"} + long := api.AgentVaultSubstitution{Placeholder: "__TOKEN__V2", Surfaces: []string{"header"}, Value: "SECRET_B"} + + for _, wire := range [][]api.AgentVaultSubstitution{{short, long}, {long, short}} { + req := requestTo(t, "GET", "/") + req.Header.Set("X-A", "__TOKEN__") + req.Header.Set("X-B", "__TOKEN__V2") + applySubstitutions(req, "svc", toSubstitutions(wire)) + + if got := req.Header.Get("X-A"); got != "SECRET_A" { + t.Errorf("server order %q: X-A = %q, want SECRET_A", wire[0].Placeholder, got) + } + if got := req.Header.Get("X-B"); got != "SECRET_B" { + t.Errorf("server order %q: X-B = %q, want SECRET_B", wire[0].Placeholder, got) + } + } +} + +// A client building the query from parameters percent-encodes the placeholder first, so both forms have to +// be matched. Underscore-style placeholders are never encoded and stand as the control. +func TestAQuerySubstitutionMatchesTheEncodedPlaceholderToo(t *testing.T) { + cases := []struct { + placeholder string + wire string + }{ + {"__PAT__", "__PAT__"}, + {"{{PAT}}", "{{PAT}}"}, + {"{{PAT}}", "%7B%7BPAT%7D%7D"}, + } + + for _, c := range cases { + req := requestTo(t, "GET", "/v1?key="+c.wire) + applySubstitutions(req, "svc", []substitution{subOn(c.placeholder, "SECRET", surfaceQuery)}) + if got := req.URL.RawQuery; got != "key=SECRET" { + t.Errorf("placeholder %q sent as %q: query = %q, want key=SECRET", c.placeholder, c.wire, got) + } + } +} + +func TestAPathSubstitutionLeavesTheRestOfThePathAlone(t *testing.T) { + for _, tc := range []struct{ name, target, wantURI string }{ + { + "an encoded slash survives", + "https://gitlab.com/api/v4/projects/group%2Fproject/repository/__PAT__", + "/api/v4/projects/group%2Fproject/repository/real", + }, + { + "an encoded plus survives", + "https://api.github.com/repos/a%2Bb/__PAT__", + "/repos/a%2Bb/real", + }, + { + "an encoded space survives", + "https://api.github.com/repos/a%20b/__PAT__", + "/repos/a%20b/real", + }, + { + "non-ASCII survives", + "https://api.github.com/repos/caf%C3%A9/__PAT__", + "/repos/caf%C3%A9/real", + }, + } { + t.Run(tc.name, func(t *testing.T) { + req, err := http.NewRequest("GET", tc.target, nil) + if err != nil { + t.Fatal(err) + } + applySubstitutions(req, "gitlab", []substitution{subOn("__PAT__", "real", surfacePath)}) + if got := req.URL.RequestURI(); got != tc.wantURI { + t.Fatalf("wire path = %q, want %q", got, tc.wantURI) + } + }) + } + + t.Run("a secret containing a slash cannot add a segment", func(t *testing.T) { + req, _ := http.NewRequest("GET", "https://api.github.com/repos/__PAT__", nil) + applySubstitutions(req, "github", []substitution{subOn("__PAT__", "a/b", surfacePath)}) + if got := req.URL.RequestURI(); got != "/repos/a%2Fb" { + t.Fatalf("wire path = %q, want the slash escaped", got) + } + }) +} + +func TestAQuerySubstitutionEscapesTheValue(t *testing.T) { + // wantRaw is asserted as well as wantKey: ParseQuery turns '+' back into a space, so a value escaped as + // form data rather than per RFC 3986 reads correctly here while going out wrong on the wire. + for _, tc := range []struct{ name, secret, wantKey, wantRaw string }{ + {"a base64 key with a plus", "aB+cD/eF==", "aB+cD/eF==", "key=aB%2BcD%2FeF%3D%3D&page=2"}, + {"a value with a space", "has space", "has space", "key=has%20space&page=2"}, + {"a value with an ampersand", "a&page=99", "a&page=99", "key=a%26page%3D99&page=2"}, + } { + t.Run(tc.name, func(t *testing.T) { + req, _ := http.NewRequest("GET", "https://api.example.com/x?key=__PAT__&page=2", nil) + applySubstitutions(req, "svc", []substitution{subOn("__PAT__", tc.secret, surfaceQuery)}) + + parsed, err := url.ParseQuery(req.URL.RawQuery) + if err != nil { + t.Fatalf("the query no longer parses: %v", err) + } + if got := parsed.Get("key"); got != tc.wantKey { + t.Fatalf("upstream reads key=%q, want %q", got, tc.wantKey) + } + if got := parsed.Get("page"); got != "2" { + t.Fatalf("the substitution disturbed another parameter: page=%q", got) + } + if req.URL.RawQuery != tc.wantRaw { + t.Fatalf("wire query = %q, want %q", req.URL.RawQuery, tc.wantRaw) + } + }) + } +} + +func TestAnEncodedPlaceholderMatchesWhateverCaseItsEscapesUse(t *testing.T) { + for _, tc := range []struct{ name, url, wantURI string }{ + {"typed", "https://api.example.com/x/{{PAT}}", "/x/SECRET"}, + {"upper escapes", "https://api.example.com/x/%7B%7BPAT%7D%7D", "/x/SECRET"}, + {"lower escapes", "https://api.example.com/x/%7b%7bPAT%7d%7d", "/x/SECRET"}, + {"query typed", "https://api.example.com/x?k={{PAT}}", "/x?k=SECRET"}, + {"query upper", "https://api.example.com/x?k=%7B%7BPAT%7D%7D", "/x?k=SECRET"}, + {"query lower", "https://api.example.com/x?k=%7b%7bPAT%7d%7d", "/x?k=SECRET"}, + } { + t.Run(tc.name, func(t *testing.T) { + surface := surfacePath + if strings.Contains(tc.url, "?") { + surface = surfaceQuery + } + req, _ := http.NewRequest("GET", tc.url, nil) + applySubstitutions(req, "svc", []substitution{subOn("{{PAT}}", "SECRET", surface)}) + if got := req.URL.RequestURI(); got != tc.wantURI { + t.Fatalf("wire = %q, want %q", got, tc.wantURI) + } + }) + } +} + +func TestTheExpansionLimitHoldsWithoutOverflowing(t *testing.T) { + if _, ok := replaceWithinLimit("aaaa", "a", strings.Repeat("x", 10), 12); ok { + t.Fatal("an expansion past the limit should be refused") + } + if got, ok := replaceWithinLimit("ab", "a", "xy", 12); !ok || got != "xyb" { + t.Fatalf("an expansion within the limit should apply, got %q ok=%v", got, ok) + } + // Shrinking never needs the limit, and must not be refused by the division branch. + if got, ok := replaceWithinLimit("aaaa", "aa", "b", 12); !ok || got != "bb" { + t.Fatalf("a shrinking replacement should apply, got %q ok=%v", got, ok) + } +} + +// Reads a prefix, then fails, the way a dropped upload does. +type truncatingBody struct { + head string + n int +} + +func (b *truncatingBody) Read(p []byte) (int, error) { + if b.n < len(b.head) { + n := copy(p, b.head[b.n:]) + b.n += n + return n, nil + } + return 0, errors.New("connection reset mid-body") +} + +func (b *truncatingBody) Close() error { return nil } + +// The safety net used to be "leave ContentLength disagreeing so http.Transport refuses". A chunked upload +// declares -1, so there was nothing to leave wrong and the upstream received a partial request with the +// credential on it, answering 200 to something the agent never finished sending. +func TestABodyThatCannotBeReadWholeIsRefusedWhateverTheEncoding(t *testing.T) { + for _, tc := range []struct { + name string + contentLength int64 + }{ + {"declared length", 1007}, + {"chunked", -1}, + } { + t.Run(tc.name, func(t *testing.T) { + req, _ := http.NewRequest("POST", "https://api.github.com/x", nil) + req.Body = &truncatingBody{head: "only the first few bytes"} + req.ContentLength = tc.contentLength + + _, err := applySubstitutions(req, "github", []substitution{subOn("__PAT__", "real", surfaceBody)}) + if !errors.Is(err, errBodyUnreadable) { + t.Fatalf("err = %v, want errBodyUnreadable", err) + } + }) + } +} + +// The path is rewritten before the body is read, so a refusal still has to record that the credential was +// written into the request. +func TestSurfacesAlreadySubstitutedSurviveABodyFailure(t *testing.T) { + req, _ := http.NewRequest("POST", "https://api.github.com/repos/__PAT__/x", nil) + req.Body = &truncatingBody{head: "only the first few bytes"} + req.ContentLength = -1 + + surfaces, err := applySubstitutions(req, "github", []substitution{subOn("__PAT__", "real", surfacePath, surfaceBody)}) + if !errors.Is(err, errBodyUnreadable) { + t.Fatalf("err = %v, want errBodyUnreadable", err) + } + if len(surfaces) == 0 || surfaces[0] != surfacePath { + t.Errorf("surfaces = %v, want the path substitution recorded", surfaces) + } +} diff --git a/packages/api/agent_vault.go b/packages/api/agent_vault.go index 8c9cbadf..9dbb4708 100644 --- a/packages/api/agent_vault.go +++ b/packages/api/agent_vault.go @@ -77,12 +77,29 @@ type AgentVaultCredential struct { Password string `json:"password,omitempty"` } +type AgentVaultCustomHeader struct { + Name string `json:"name"` + Prefix string `json:"prefix,omitempty"` + Value string `json:"value"` +} + +type AgentVaultSubstitution struct { + Placeholder string `json:"placeholder"` + Surfaces []string `json:"surfaces"` + Value string `json:"value"` +} + type AgentVaultService struct { - ID string `json:"id"` - Name string `json:"name"` - AccessBundleName string `json:"accessBundleName"` - HostPattern string `json:"hostPattern"` - Credential AgentVaultCredential `json:"credential"` + ID string `json:"id"` + Name string `json:"name"` + AccessBundleName string `json:"accessBundleName"` + HostPattern string `json:"hostPattern"` + // A nil slice means unrestricted, which is what JSON null decodes to. + AllowedMethods []string `json:"allowedMethods"` + AllowedPathPrefixes []string `json:"allowedPathPrefixes"` + Credential AgentVaultCredential `json:"credential"` + CustomHeaders []AgentVaultCustomHeader `json:"customHeaders"` + Substitutions []AgentVaultSubstitution `json:"substitutions"` } type ResolveAgentVaultSessionResponse struct {