diff --git a/agent/server/grpctunnel/conformance_test.go b/agent/server/grpctunnel/conformance_test.go index 739a211..410053a 100644 --- a/agent/server/grpctunnel/conformance_test.go +++ b/agent/server/grpctunnel/conformance_test.go @@ -57,9 +57,11 @@ type conformanceCase struct { URL string `json:"url"` Headers map[string]string `json:"headers"` // Code is the CallCancel status a rejected request must carry. - // Defaults to 404 (no rule matched); 400 says the request was - // malformed before matching — bad encoding, directory traversal. + // Defaults to 404 (no rule matched); 403 says a rule matched but did + // not authorize the destination the request named. Code int32 `json:"code"` + // AbsentHeaders names headers that must not reach the upstream. + AbsentHeaders []string `json:"absentHeaders"` } `json:"expect"` } @@ -93,7 +95,8 @@ func runConformanceFixture(t *testing.T, path string) { rules = append(rules, r) } } - router := NewRouter(rules, zap.NewNop()) + router, err := NewRouter(rules, zap.NewNop()) + require.NoError(t, err) for _, c := range fixture.Cases { t.Run(c.Name, func(t *testing.T) { @@ -127,6 +130,9 @@ func runConformanceFixture(t *testing.T, path string) { for k, v := range c.Expect.Headers { assert.Equal(t, v, breq.Header.Get(k), "outgoing header %q", k) } + for _, k := range c.Expect.AbsentHeaders { + assert.Empty(t, breq.Header.Values(k), "header %q must not reach the upstream", k) + } }) } } diff --git a/agent/server/grpctunnel/router.go b/agent/server/grpctunnel/router.go index 04e4033..8fb1b57 100644 --- a/agent/server/grpctunnel/router.go +++ b/agent/server/grpctunnel/router.go @@ -28,9 +28,15 @@ type Router struct { inner *acceptfile.Router } -// NewRouter creates a Router over rendered accept file rules. -func NewRouter(rules []acceptfile.AcceptFileRuleWrapper, logger *zap.Logger) *Router { - return &Router{inner: acceptfile.NewRouter(rules, logger)} +// NewRouter creates a Router over rendered accept file rules. It fails when a +// rule's origin cannot be resolved into a policy — a malformed wildcard, an +// undialable port — so the agent stops at startup rather than per request. +func NewRouter(rules []acceptfile.AcceptFileRuleWrapper, logger *zap.Logger) (*Router, error) { + inner, err := acceptfile.NewRouter(rules, logger) + if err != nil { + return nil, err + } + return &Router{inner: inner}, nil } // Route resolves a CallStart to a BackendRequest, or a *RouteError with an @@ -47,6 +53,9 @@ func (rt *Router) Route(start *pb.CallStart) (*BackendRequest, error) { return nil, &RouteError{Code: http.StatusNotFound, Reason: err.Error()} case errors.As(err, &invalid): return nil, &RouteError{Code: http.StatusBadRequest, Reason: invalid.Reason} + case errors.Is(err, acceptfile.ErrDestinationRejected): + // The reason names the policy, never the value that failed it. + return nil, &RouteError{Code: http.StatusForbidden, Reason: err.Error()} default: return nil, &RouteError{Code: http.StatusBadGateway, Reason: err.Error()} } diff --git a/agent/server/grpctunnel/router_test.go b/agent/server/grpctunnel/router_test.go index db90192..9cf7c90 100644 --- a/agent/server/grpctunnel/router_test.go +++ b/agent/server/grpctunnel/router_test.go @@ -40,6 +40,15 @@ func makeRouterRules(t *testing.T, rules string) []acceptfile.AcceptFileRuleWrap return filtered } +// newRouter builds a Router over the rules, failing the test if the accept +// file declares a policy the Router refuses. +func newRouter(t *testing.T, rulesJSON string) *Router { + t.Helper() + router, err := NewRouter(makeRouterRules(t, rulesJSON), zap.NewNop()) + require.NoError(t, err) + return router +} + func callStart(method, path string, headers map[string]string) *pb.CallStart { return &pb.CallStart{ PseudoHeaders: map[string]string{":method": method, ":path": path}, @@ -84,7 +93,7 @@ func TestRouterBackend_BasicRequest(t *testing.T) { ] }`, server.URL) - router := NewRouter(makeRouterRules(t, rulesJSON), zap.NewNop()) + router := newRouter(t, rulesJSON) status, body, headers := doCall(t, router, callStart("GET", "/api/v1/repos", nil), nil) assert.Equal(t, http.StatusOK, status) assert.Equal(t, `{"repos": []}`, body) @@ -103,7 +112,7 @@ func TestRouterBackend_QueryStringForwarded(t *testing.T) { "private": [{"method": "GET", "path": "/api/*", "origin": "%s"}] }`, server.URL) - router := NewRouter(makeRouterRules(t, rulesJSON), zap.NewNop()) + router := newRouter(t, rulesJSON) status, _, _ := doCall(t, router, callStart("GET", "/api/search?q=foo&page=2", nil), nil) assert.Equal(t, http.StatusOK, status) } @@ -120,7 +129,7 @@ func TestRouterBackend_EncodedSlashPreserved(t *testing.T) { "private": [{"method": "any", "path": "/api/*", "origin": "%s"}] }`, server.URL) - router := NewRouter(makeRouterRules(t, rulesJSON), zap.NewNop()) + router := newRouter(t, rulesJSON) status, _, _ := doCall(t, router, callStart("GET", "/api/v4/projects/group%2Fproject", nil), nil) assert.Equal(t, http.StatusOK, status) } @@ -136,7 +145,7 @@ func TestRouter_NoMatchingRule(t *testing.T) { ] }` - router := NewRouter(makeRouterRules(t, rulesJSON), zap.NewNop()) + router := newRouter(t, rulesJSON) _, err := router.Route(callStart("GET", "/unknown/path", nil)) require.Error(t, err) var re *RouteError @@ -167,7 +176,7 @@ func TestRouterBackend_BearerAuth(t *testing.T) { ] }`, server.URL) - router := NewRouter(makeRouterRules(t, rulesJSON), zap.NewNop()) + router := newRouter(t, rulesJSON) status, _, _ := doCall(t, router, callStart("GET", "/api/repos", nil), nil) assert.Equal(t, http.StatusOK, status) } @@ -197,7 +206,7 @@ func TestRouterBackend_BasicAuth(t *testing.T) { ] }`, server.URL) - router := NewRouter(makeRouterRules(t, rulesJSON), zap.NewNop()) + router := newRouter(t, rulesJSON) status, _, _ := doCall(t, router, callStart("POST", "/api/data", nil), strings.NewReader(`{"key":"value"}`)) assert.Equal(t, http.StatusOK, status) } @@ -222,7 +231,7 @@ func TestRouterBackend_RuleHeaderInjection(t *testing.T) { ] }`, server.URL) - router := NewRouter(makeRouterRules(t, rulesJSON), zap.NewNop()) + router := newRouter(t, rulesJSON) status, _, _ := doCall(t, router, callStart("GET", "/api/repos", map[string]string{"x-custom": "caller-value"}), nil) assert.Equal(t, http.StatusOK, status) } @@ -250,7 +259,7 @@ func TestRouterBackend_StreamedRequestBody(t *testing.T) { bodyW.Close() }() - router := NewRouter(makeRouterRules(t, rulesJSON), zap.NewNop()) + router := newRouter(t, rulesJSON) status, _, _ := doCall(t, router, callStart("POST", "/api/upload", nil), bodyR) assert.Equal(t, http.StatusOK, status) } diff --git a/agent/server/grpctunnel/tunnel_client.go b/agent/server/grpctunnel/tunnel_client.go index cff87ab..2a2f44a 100644 --- a/agent/server/grpctunnel/tunnel_client.go +++ b/agent/server/grpctunnel/tunnel_client.go @@ -474,7 +474,11 @@ func (tc *tunnelClient) setupRouter() error { if err != nil { return fmt.Errorf("error parsing rendered accept file: %w", err) } - tc.router = NewRouter(af2.Wrapper().PrivateRules(), tc.logger) + router, err := NewRouter(af2.Wrapper().PrivateRules(), tc.logger) + if err != nil { + return fmt.Errorf("error building accept file router: %w", err) + } + tc.router = router return nil } diff --git a/agent/server/grpctunnel/tunnel_client_test.go b/agent/server/grpctunnel/tunnel_client_test.go index 3189bd9..472f993 100644 --- a/agent/server/grpctunnel/tunnel_client_test.go +++ b/agent/server/grpctunnel/tunnel_client_test.go @@ -109,7 +109,9 @@ func catchAllRouter(t *testing.T) *Router { "private": [{"method": "any", "path": "/*", "origin": "http://stub.internal"}] }`), cfg, zap.NewNop()) require.NoError(t, err) - return NewRouter(af.Wrapper().PrivateRules(), zap.NewNop()) + router, err := NewRouter(af.Wrapper().PrivateRules(), zap.NewNop()) + require.NoError(t, err) + return router } // ----------------------------------------------------------------------------- diff --git a/agent/server/snykbroker/acceptfile/accept_file.go b/agent/server/snykbroker/acceptfile/accept_file.go index 0713bf3..eabed57 100644 --- a/agent/server/snykbroker/acceptfile/accept_file.go +++ b/agent/server/snykbroker/acceptfile/accept_file.go @@ -3,7 +3,6 @@ package acceptfile import ( "encoding/json" "fmt" - "net/url" "os" "sync" @@ -234,21 +233,22 @@ func (r AcceptFileRuleWrapper) Origin() string { return "" } origin := os.ExpandEnv(rawOrigin) - asUrl, err := url.Parse(origin) + withScheme, err := defaultScheme(origin) if err != nil { r.acceptFile.logger.Panic("failed to parse origin URL", zap.String("origin", rawOrigin), zap.Error(err)) } - if asUrl.Scheme == "" { + if withScheme != origin { if _, seen := r.acceptFile.schemeWarned.LoadOrStore(rawOrigin, struct{}{}); !seen { r.acceptFile.logger.Debug("origin URL has no scheme, defaulting to https", zap.String("origin", rawOrigin)) } - asUrl.Scheme = "https" - return asUrl.String() } - return origin - + return withScheme } +// Path returns the rule's path pattern as written, ${VAR} references included. +// Expanding them here would pin the segment to the configured value; matchPath +// instead treats ${VAR} as snyk-broker does, matching any one segment and +// substituting the configured value into the outgoing URL. func (r AcceptFileRuleWrapper) Path() string { path, ok := r.dict["path"].(string) if !ok { @@ -257,6 +257,15 @@ func (r AcceptFileRuleWrapper) Path() string { return path } +// RawOrigin returns the rule's origin exactly as written, with ${VAR} +// references intact. The Router needs this because pool rotation has to happen +// on the reference: expanding ${API} against the environment first turns a +// pool-only variable into the empty string, leaving nothing to rotate. +func (r AcceptFileRuleWrapper) RawOrigin() string { + origin, _ := r.dict["origin"].(string) + return origin +} + func (r AcceptFileRuleWrapper) SetOrigin(origin string) { r.dict["origin"] = origin } diff --git a/agent/server/snykbroker/acceptfile/auth.go b/agent/server/snykbroker/acceptfile/auth.go new file mode 100644 index 0000000..d6949c9 --- /dev/null +++ b/agent/server/snykbroker/acceptfile/auth.go @@ -0,0 +1,98 @@ +package acceptfile + +import ( + "encoding/base64" + "net/http" + "os" + "sort" + "strings" +) + +// authScheme is an accept-file auth scheme, always lowercased. Go has no enum, +// but a named type keeps the scheme from being just another string in flight +// and lets the declaration below and its uses refer to the same symbols. +type authScheme string + +const ( + authSchemeBearer authScheme = "bearer" + authSchemeToken authScheme = "token" + authSchemeBasic authScheme = "basic" + authSchemeRaw authScheme = "raw" +) + +// authHeaderBuilders is the single declaration of which schemes Axon supports: +// a scheme is supported exactly when there is a builder for it here. +// +// The list and the behaviour have to be one table, not two. They were a set in +// supported.go and a switch in router.go, which could drift in either +// direction — a scheme in the set with no case sends no credential while +// claiming to be supported, and a case with no set entry warns about itself +// while working fine. +// +// The bodies match snyk-broker's lib/common/utils/auth-header.ts. +var authHeaderBuilders = map[authScheme]func(auth *AcceptFileRuleAuth) string{ + authSchemeBearer: func(auth *AcceptFileRuleAuth) string { + return "Bearer " + os.ExpandEnv(auth.Token) + }, + authSchemeToken: func(auth *AcceptFileRuleAuth) string { + return "Token " + os.ExpandEnv(auth.Token) + }, + // The upstream's header carries no scheme prefix at all. + authSchemeRaw: func(auth *AcceptFileRuleAuth) string { + return os.ExpandEnv(auth.Token) + }, + authSchemeBasic: func(auth *AcceptFileRuleAuth) string { + return "Basic " + basicCredential(auth) + }, +} + +// authHeaderBuilder returns how to build the header for a scheme as written in +// the accept file, and whether the scheme is one Axon supports at all. +func authHeaderBuilder(scheme string) (func(*AcceptFileRuleAuth) string, bool) { + build, ok := authHeaderBuilders[authScheme(strings.ToLower(scheme))] + return build, ok +} + +// isSupportedAuthScheme reports whether the Router can build a credential for +// the scheme as the accept file spells it. +func isSupportedAuthScheme(scheme string) bool { + _, ok := authHeaderBuilder(scheme) + return ok +} + +// supportedAuthSchemes lists the schemes in a stable order, for a message that +// has to name them. +func supportedAuthSchemes() []string { + names := make([]string, 0, len(authHeaderBuilders)) + for scheme := range authHeaderBuilders { + names = append(names, string(scheme)) + } + sort.Strings(names) + return names +} + +// applyAuth sets the Authorization header from the rule's auth block. +// +// An unrecognized scheme sets no header at all, which is what snyk-broker's +// authHeader() does with one; warnUnsupportedRule has already said so when the +// Router was built. +func applyAuth(header http.Header, auth *AcceptFileRuleAuth) { + if auth == nil { + return + } + if build, ok := authHeaderBuilder(auth.Scheme); ok { + header.Set("Authorization", build(auth)) + } +} + +// basicCredential builds the base64 payload. A basic block carrying "token" +// holds the already-encoded user:pass pair (the shape Azure Repos uses), so +// encoding it again would send a broken credential. +func basicCredential(auth *AcceptFileRuleAuth) string { + if auth.Username != "" || auth.Password != "" { + username := os.ExpandEnv(auth.Username) + password := os.ExpandEnv(auth.Password) + return base64.StdEncoding.EncodeToString([]byte(username + ":" + password)) + } + return os.ExpandEnv(auth.Token) +} diff --git a/agent/server/snykbroker/acceptfile/headers.go b/agent/server/snykbroker/acceptfile/headers.go new file mode 100644 index 0000000..2e0896d --- /dev/null +++ b/agent/server/snykbroker/acceptfile/headers.go @@ -0,0 +1,58 @@ +package acceptfile + +import "strings" + +// Request-header helpers shared by rule matching and routing. None of this is +// origin policy or match semantics; it is just the handful of things that have +// to be true when headers arrive as a map with whatever casing the caller used. + +// getHeaderCaseInsensitive retrieves a header value with case-insensitive key +// matching. +func getHeaderCaseInsensitive(headers map[string]string, key string) (string, bool) { + if headers == nil { + return "", false + } + + // Try exact match first. + if v, ok := headers[key]; ok { + return v, true + } + + keyLower := strings.ToLower(key) + for k, v := range headers { + if strings.ToLower(k) == keyLower { + return v, true + } + } + + return "", false +} + +// isWebSocketUpgrade reports whether the request headers ask for an upgrade. +func isWebSocketUpgrade(headers map[string]string) bool { + upgrade, ok := getHeaderCaseInsensitive(headers, "Upgrade") + if !ok { + return false + } + return strings.EqualFold(strings.TrimSpace(upgrade), "websocket") +} + +// takeTargetHosts removes every spelling of the target-host header, returning +// the values it carried and the headers without it. Callers get a copy, so the +// caller's own map is left alone. +// +// Every spelling, and all of them: a map can hold the header twice under +// different casing, and two values is a shape the destination policy refuses +// rather than picks from. +func takeTargetHosts(headers map[string]string) ([]string, map[string]string) { + var values []string + remaining := make(map[string]string, len(headers)) + for k, v := range headers { + if strings.EqualFold(k, HeaderTargetHost) { + values = append(values, v) + continue + } + remaining[k] = v + } + return values, remaining +} diff --git a/agent/server/snykbroker/acceptfile/helpers_test.go b/agent/server/snykbroker/acceptfile/helpers_test.go index e3ba270..bd370b1 100644 --- a/agent/server/snykbroker/acceptfile/helpers_test.go +++ b/agent/server/snykbroker/acceptfile/helpers_test.go @@ -1,16 +1,25 @@ package acceptfile import ( + "testing" + "github.com/cortexapps/axon/config" "github.com/stretchr/testify/require" "go.uber.org/zap" - - "testing" ) // newTestRouter builds a Router the way the tunnel client does: parse, render, // re-parse, take the private rules minus the injected /__axon/* self-route. func newTestRouter(t *testing.T, rulesJSON string, pluginDirs ...string) *Router { + t.Helper() + rt, err := newTestRouterErr(t, rulesJSON, pluginDirs...) + require.NoError(t, err) + return rt +} + +// newTestRouterErr is the same, for the cases that are about the Router +// refusing to be built at all. +func newTestRouterErr(t *testing.T, rulesJSON string, pluginDirs ...string) (*Router, error) { t.Helper() if pluginDirs == nil { pluginDirs = []string{} @@ -18,11 +27,17 @@ func newTestRouter(t *testing.T, rulesJSON string, pluginDirs ...string) *Router cfg := config.AgentConfig{HttpServerPort: 8080, PluginDirs: pluginDirs} af, err := NewAcceptFile([]byte(rulesJSON), cfg, zap.NewNop()) - require.NoError(t, err) + if err != nil { + return nil, err + } rendered, err := af.Render(zap.NewNop()) - require.NoError(t, err) + if err != nil { + return nil, err + } af2, err := NewAcceptFile(rendered, cfg, zap.NewNop()) - require.NoError(t, err) + if err != nil { + return nil, err + } var rules []AcceptFileRuleWrapper for _, r := range af2.Wrapper().PrivateRules() { diff --git a/agent/server/snykbroker/acceptfile/matcher.go b/agent/server/snykbroker/acceptfile/matcher.go index 06c8423..735a03f 100644 --- a/agent/server/snykbroker/acceptfile/matcher.go +++ b/agent/server/snykbroker/acceptfile/matcher.go @@ -69,28 +69,6 @@ func matchesValid(requirements []ValidHeaderRequirement, headers map[string]stri return true } -// getHeaderCaseInsensitive retrieves a header value with case-insensitive key matching. -func getHeaderCaseInsensitive(headers map[string]string, key string) (string, bool) { - if headers == nil { - return "", false - } - - // Try exact match first. - if v, ok := headers[key]; ok { - return v, true - } - - // Case-insensitive search. - keyLower := strings.ToLower(key) - for k, v := range headers { - if strings.ToLower(k) == keyLower { - return v, true - } - } - - return "", false -} - // matchesMethod checks if the rule method matches the request method. // "any" matches all methods. A rule with no method means GET, as it does in // snyk-broker — leaving it unmatched would make the rule silently dead. diff --git a/agent/server/snykbroker/acceptfile/origin.go b/agent/server/snykbroker/acceptfile/origin.go new file mode 100644 index 0000000..f2f6a8d --- /dev/null +++ b/agent/server/snykbroker/acceptfile/origin.go @@ -0,0 +1,212 @@ +package acceptfile + +import ( + "errors" + "fmt" + "net" + "net/url" + "strconv" + "strings" + "sync" + + "golang.org/x/net/publicsuffix" +) + +// HeaderTargetHost is routing input, never authorization: the origin declared +// in the accept file stays the policy, and this value is only ever checked +// against it. +const HeaderTargetHost = "x-cortex-target-host" + +// ErrDestinationRejected marks a request that matched a rule but named a +// destination the rule does not authorize. Transports map it to 403. +var ErrDestinationRejected = errors.New("destination rejected") + +// wildcardOrigin is the family of hosts a wildcard origin authorizes. +type wildcardOrigin struct { + // Keeps the leading dot, so matching is label-aligned: ".example.com". + suffix string +} + +// matches reports whether host sits exactly one label under the wildcard. +// +// One label, not one-or-more: a wildcard certificate covers a single label, so +// a multi-label match would authorize names that verification then refuses. Do +// not relax this to a plain suffix test. +func (w wildcardOrigin) matches(host string) bool { + if !strings.HasSuffix(host, w.suffix) { + return false + } + label := host[:len(host)-len(w.suffix)] + return label != "" && !strings.Contains(label, ".") +} + +// parsedOrigin is a rule's origin after parsing: the URL to dial, and the +// family it authorizes when the operator wrote a wildcard. The two always +// travel together — every destination decision needs both, since a nil wildcard +// is what makes an origin concrete. +type parsedOrigin struct { + url *url.URL + wildcard *wildcardOrigin +} + +// isWildcard reports whether the origin authorizes a family rather than naming +// one host. +func (p parsedOrigin) isWildcard() bool { return p.wildcard != nil } + +// parsedOrigins caches parseOrigin by origin string. A rotating pool resolves to +// a handful of distinct values, so this keeps the per-request cost to a map +// lookup rather than a URL parse plus a public-suffix walk. +var parsedOrigins sync.Map + +type originCacheEntry struct { + parsed parsedOrigin + err error +} + +// parseOrigin returns the URL to dial and, for a wildcard origin, the family it +// authorizes. Routers run it at construction so a malformed policy fails there +// rather than per request. +func parseOrigin(origin string) (parsedOrigin, error) { + if cached, ok := parsedOrigins.Load(origin); ok { + entry := cached.(originCacheEntry) + return entry.parsed, entry.err + } + parsed, err := parseOriginUncached(origin) + parsedOrigins.Store(origin, originCacheEntry{parsed: parsed, err: err}) + return parsed, err +} + +func parseOriginUncached(origin string) (parsedOrigin, error) { + asURL, err := url.Parse(origin) + if err != nil { + return parsedOrigin{}, fmt.Errorf("invalid origin %q: %w", origin, err) + } + + if !strings.Contains(asURL.Host, "*") { + // Deliberately not port-checked. A concrete origin names one host the + // operator picked, and the agent's own origin carries port 0 until its + // listener binds. + return parsedOrigin{url: asURL}, nil + } + + // Certificate verification is the destination control, so a family cannot + // be served over plaintext. + if asURL.Scheme != "https" { + return parsedOrigin{}, fmt.Errorf("wildcard origin %q must use https", origin) + } + // A wildcard origin's port is dialed against a host the operator never + // wrote down, so an unusable one has to stop the agent rather than surface + // as a per-request failure. + if err := validatePort(asURL.Port()); err != nil { + return parsedOrigin{}, fmt.Errorf("wildcard origin %q: %w", origin, err) + } + + host := asURL.Hostname() + suffix, found := strings.CutPrefix(host, "*.") + if !found || strings.Contains(suffix, "*") { + return parsedOrigin{}, fmt.Errorf("wildcard origin %q must have the form https://*.example.com", origin) + } + // "https://*." alone would authorize every host, and an empty label would + // misalign the match against the dot this stores. + if suffix == "" || strings.HasPrefix(suffix, ".") || + strings.HasSuffix(suffix, ".") || strings.Contains(suffix, "..") { + return parsedOrigin{}, fmt.Errorf("wildcard origin %q has an empty label", origin) + } + // ICANN division only. The private section holds ordinary registrable + // domains added for cookie scoping, and rejecting those would rule out + // legitimate families. + if publicSuffix, icann := publicsuffix.PublicSuffix(suffix); icann && publicSuffix == suffix { + return parsedOrigin{}, fmt.Errorf("wildcard origin %q must contain a registrable domain", origin) + } + + return parsedOrigin{url: asURL, wildcard: &wildcardOrigin{suffix: "." + suffix}}, nil +} + +// parseTargetHost normalizes a header value into a host to match against the +// origin, or reports why it is unusable. +// +// Deliberately not a hostname validator. The destination controls are +// wildcardOrigin.matches, which confines the value to the family whatever it +// contains, and the certificate check behind it, which no made-up name can +// pass. What is left here is the handful of shapes that would confuse those two +// or dial something other than a host in the family. +// +// The value names a host and nothing else. The origin decides the port, so a +// value carrying one is either restating it or trying to change it, and there +// is no reason to tell those apart. +func parseTargetHost(value string) (string, error) { + if value == "" { + return "", fmt.Errorf("empty target host") + } + // A comma-joined value would otherwise pass the suffix test whole and come + // back as the host to dial, so this guards the match rather than tidying + // input. + if strings.ContainsAny(value, ",") { + return "", fmt.Errorf("target host carries multiple values") + } + // This becomes a Host header, so CR and LF do not get to travel. + if strings.TrimSpace(value) != value || strings.ContainsAny(value, " \t\r\n") { + return "", fmt.Errorf("target host contains whitespace") + } + if _, _, err := net.SplitHostPort(value); err == nil { + return "", fmt.Errorf("target host declares a port") + } + + // Lowercased because matches and the concrete-origin comparison are both + // byte comparisons against a policy the operator wrote in some other case. + host := strings.ToLower(value) + // An address cannot sit under a DNS family, so one here means the caller is + // trying to leave the policy rather than move within it. + if net.ParseIP(host) != nil { + return "", fmt.Errorf("target host is an IP address") + } + return host, nil +} + +// validatePort accepts an empty port, meaning the scheme's default. url.Parse +// already rejects a non-numeric port, but not 0 or an out-of-range number. +func validatePort(port string) error { + if port == "" { + return nil + } + n, err := strconv.Atoi(port) + if err != nil || n < 1 || n > 65535 { + return fmt.Errorf("port %q is not in 1-65535", port) + } + return nil +} + +// resolveTargetHost returns the host to retarget to, or "" to dial the origin +// as declared. Fail closed both ways: a family never falls back to a declared +// host, and a concrete origin never ignores a value that disagrees with it. +func (p parsedOrigin) resolveTargetHost(values []string) (string, error) { + if len(values) > 1 { + return "", fmt.Errorf("%w: duplicate target host", ErrDestinationRejected) + } + + if len(values) == 0 { + if p.isWildcard() { + return "", fmt.Errorf("%w: wildcard origin requires a target host", ErrDestinationRejected) + } + return "", nil + } + + host, err := parseTargetHost(values[0]) + if err != nil { + return "", fmt.Errorf("%w: %s", ErrDestinationRejected, err) + } + + if p.isWildcard() { + if !p.wildcard.matches(host) { + return "", fmt.Errorf("%w: target host is outside the origin policy", ErrDestinationRejected) + } + return host, nil + } + + // A concrete origin routes on itself, but a value that names somewhere else + // is a request to go there — refuse rather than silently ignore it. + if host != strings.ToLower(p.url.Hostname()) { + return "", fmt.Errorf("%w: target host disagrees with the origin", ErrDestinationRejected) + } + return "", nil +} diff --git a/agent/server/snykbroker/acceptfile/origin_contract_test.go b/agent/server/snykbroker/acceptfile/origin_contract_test.go index ff9a174..c30a241 100644 --- a/agent/server/snykbroker/acceptfile/origin_contract_test.go +++ b/agent/server/snykbroker/acceptfile/origin_contract_test.go @@ -11,11 +11,13 @@ import ( // AcceptFileRuleWrapper.Origin() is the one accessor this package shares with // the snyk-broker reflector: relay_instance_manager.go reads it to decide // whether a rule needs a wildcard policy, to build the reflector proxy URI, and -// to report a bad origin. Nothing else in here is reachable from that path — -// Path() and MatchRule have no callers outside this package and grpctunnel. +// to report a bad origin. Nothing else the tunnel work touched is reachable +// from that path — Path() and MatchRule have no callers outside this package +// and grpctunnel. // -// It is pinned here, before the routing work that follows refactors it, so a -// change to the shared accessor cannot quietly alter what the reflector sees. +// So this pins Origin()'s contract directly, in the shapes the reflector +// depends on, rather than leaving it implied by tests of the things built on +// top of it. func TestOriginContract(t *testing.T) { cases := []struct { name string @@ -116,4 +118,5 @@ func TestOriginOfRuleWithoutOneIsEmpty(t *testing.T) { rules := af.Wrapper().PrivateRules() require.Len(t, rules, 1) require.Equal(t, "", rules[0].Origin()) + require.Equal(t, "", rules[0].RawOrigin()) } diff --git a/agent/server/snykbroker/acceptfile/origin_test.go b/agent/server/snykbroker/acceptfile/origin_test.go new file mode 100644 index 0000000..f67e46b --- /dev/null +++ b/agent/server/snykbroker/acceptfile/origin_test.go @@ -0,0 +1,180 @@ +package acceptfile + +import ( + "strings" + "testing" + + "github.com/stretchr/testify/require" +) + +// These mirror the reflector's wildcard-origin suite +// (server/snykbroker/reflector_dynamic_target_test.go) case for case, against +// the Router's own implementation. The Router is the authoritative accept-file +// engine; the reflector keeps its copy until it is rerouted through this one, +// and until then the two suites are how we know they agree. + +func TestParseOriginAcceptsWildcardFamilies(t *testing.T) { + for _, origin := range []string{ + "https://*.api.example.net", + "https://*.internal.api.example.net", + "https://*.axon.example.com", + "https://*.api.example.net:8443", + "https://*.something.com.internal:8443", + "https://*.googleapis.com", + } { + parsed, err := parseOrigin(origin) + require.NoError(t, err, "origin=%q", origin) + require.True(t, parsed.isWildcard(), "origin=%q", origin) + } +} + +func TestParseOriginRejectsUnusableWildcards(t *testing.T) { + cases := map[string]string{ + "plaintext": "http://*.api.example.net", + "bare wildcard": "https://*", + "partial label": "https://a*.api.example.net", + "non-leftmost": "https://foo.*.api.example.net", + "two wildcards": "https://*.*.api.example.net", + "public suffix": "https://*.com", + "multipart public sfx": "https://*.co.uk", + "empty suffix": "https://*.", + "port zero": "https://*.api.example.net:0", + "port out of range": "https://*.api.example.net:70000", + } + for name, origin := range cases { + t.Run(name, func(t *testing.T) { + _, err := parseOrigin(origin) + require.Error(t, err) + }) + } +} + +func TestParseOriginLeavesConcreteOriginsAlone(t *testing.T) { + for _, origin := range []string{ + "https://beta.api.example.net", + "http://127.0.0.1:8080", + "https://api.github.com/v3", + } { + parsed, err := parseOrigin(origin) + require.NoError(t, err, "origin=%q", origin) + require.False(t, parsed.isWildcard(), "origin=%q", origin) + require.NotNil(t, parsed.url) + } +} + +// A wildcard certificate covers a single label, so a multi-label match would +// authorize names that verification then refuses. If this fails, the fix is +// not to relax it. +func TestWildcardMatchesExactlyOneLabel(t *testing.T) { + parsed, err := parseOrigin("https://*.api.example.net") + require.NoError(t, err) + wildcard := parsed.wildcard + + for _, host := range []string{ + "alpha.api.example.net", + "eu-west1-compute.api.example.net", + } { + require.True(t, wildcard.matches(host), "host=%q", host) + } + + for _, host := range []string{ + "a.b.api.example.net", + "svc.internal.api.example.net", + "api.example.net", + "evilapi.example.net", + "notapi.example.net", + "alpha.api.example.net.evil.com", + "evil.com", + } { + require.False(t, wildcard.matches(host), "host=%q", host) + } +} + +// parseTargetHost is not a hostname validator, so this covers only what it +// still claims: the shapes that would confuse the origin match or dial +// something other than a host. Everything else is the match's job, and +// TestPolicyRefusesValuesOutsideTheFamily covers that. +func TestParseTargetHostNormalizesAndRejects(t *testing.T) { + host, err := parseTargetHost("ALPHA.API.Example.NET") + require.NoError(t, err) + require.Equal(t, "alpha.api.example.net", host) + + cases := map[string]string{ + "empty": "", + "comma joined": "alpha.api.example.net,beta.api.example.net", + "leading space": " alpha.api.example.net", + "inner space": "alpha api.example.net", + "tab": "alpha.api.example.net\t", + "carriage": "alpha.api.example.net\r\nX-Evil: y", + "default port": "alpha.api.example.net:443", + "other port": "alpha.api.example.net:8443", + "ipv4": "127.0.0.1", + "ipv6": "::1", + } + for name, value := range cases { + t.Run(name, func(t *testing.T) { + _, err := parseTargetHost(value) + require.Error(t, err) + }) + } +} + +// Ordinary hostnames must reach the dial. The double-hyphen cases are here on +// purpose: they are valid DNS but idna.Lookup rejects a hyphen in label +// positions 3-4, which IDNA reserves for "xn--". If a strict validator ever +// arrives, this is what catches it refusing a real destination. +func TestPolicyAdmitsOrdinaryHostnames(t *testing.T) { + parsed, err := parseOrigin("https://*.example.net") + require.NoError(t, err) + wildcard := parsed.wildcard + + for _, value := range []string{ + "alpha.example.net", + "eu-west1-compute.example.net", + "my-service-01.example.net", + "ab--cd.example.net", + "x1--y.example.net", + "a--b.example.net", + "1.example.net", + "xn--e1afmkfd.example.net", + strings.Repeat("a", 63) + ".example.net", + } { + host, err := parseTargetHost(value) + require.NoError(t, err, "value=%q", value) + require.True(t, wildcard.matches(host), "value=%q", value) + } +} + +// The origin match is the destination control, not parseTargetHost, so these +// have to be refused by the policy however well-formed they look. +func TestPolicyRefusesValuesOutsideTheFamily(t *testing.T) { + parsed, err := parseOrigin("https://*.api.example.net") + require.NoError(t, err) + wildcard := parsed.wildcard + + cases := map[string]string{ + "other family": "alpha.evil.example.net", + "suffix as prefix": "alpha.api.example.net.evil.com", + "parent of family": "api.example.net", + "partial label": "evilapi.example.net", + "two labels": "a.b.api.example.net", + "nested family": "svc.internal.api.example.net", + "bare suffix": ".api.example.net", + "empty inner label": "alpha..api.example.net", + "trailing dot": "alpha.api.example.net.", + "path appended": "alpha.api.example.net/v1", + "url": "https://alpha.api.example.net", + "percent encoded": "alpha%2eapi.example.net", + "unicode label": "alpha.api.examplı.net", + "over-long label": strings.Repeat("a", 64) + ".example.net", + } + for name, value := range cases { + t.Run(name, func(t *testing.T) { + host, err := parseTargetHost(value) + if err != nil { + return // rejected before the policy ever saw it + } + require.False(t, wildcard.matches(host), "value=%q must not match the family", value) + }) + } +} diff --git a/agent/server/snykbroker/acceptfile/pool.go b/agent/server/snykbroker/acceptfile/pool.go index 2111c13..bef5b87 100644 --- a/agent/server/snykbroker/acceptfile/pool.go +++ b/agent/server/snykbroker/acceptfile/pool.go @@ -73,26 +73,45 @@ func (pe *poolEntry) Next() string { return pe.values[idx%uint64(len(pe.values))] } +// Peek returns the value Next would return, without consuming the slot. +// Startup validation uses it so checking a pool does not shift which member the +// first request lands on. +func (pe *poolEntry) Peek() string { + idx := pe.counter.Load() + return pe.values[idx%uint64(len(pe.values))] +} + // reEnvVar matches ${VAR_NAME} patterns in strings. var reEnvVar = regexp.MustCompile(`\$\{([^}]+)\}`) // ResolvePoolVars resolves any ${VAR} references in the string, checking for // _POOL variants first (round-robin), then falling back to regular env vars. func (pm *PoolManager) ResolvePoolVars(s string) string { + return pm.resolveVars(s, true) +} + +// resolveVars resolves ${VAR} references. advance is false for startup +// validation, which must observe a pool without consuming a slot. +// +// The pool is checked before the plain environment variable, matching +// snyk-broker's replace(). Resolution has to happen on the reference rather +// than on an already-expanded string: a variable that exists only as VAR_POOL +// expands to the empty string, leaving nothing to rotate. +func (pm *PoolManager) resolveVars(s string, advance bool) string { return reEnvVar.ReplaceAllStringFunc(s, func(match string) string { varName := match[2 : len(match)-1] // strip ${ and } - // Check pool first. if entry := pm.getPool(varName); entry != nil { - return entry.Next() + if advance { + return entry.Next() + } + return entry.Peek() } - // Fall back to regular env var. if val := os.Getenv(varName); val != "" { return val } - // Check if the value itself (already expanded) is a comma-separated pool. return match }) } diff --git a/agent/server/snykbroker/acceptfile/router.go b/agent/server/snykbroker/acceptfile/router.go index 55d0a1a..c763682 100644 --- a/agent/server/snykbroker/acceptfile/router.go +++ b/agent/server/snykbroker/acceptfile/router.go @@ -1,12 +1,10 @@ package acceptfile import ( - "encoding/base64" "errors" "fmt" "net/http" "net/url" - "os" "strings" "go.uber.org/zap" @@ -17,7 +15,8 @@ import ( var ErrNoRoute = errors.New("no matching accept file rule") // InvalidRequestError marks a request that could not be routed because it -// was malformed (bad encoding, missing fields) rather than unmatched. +// was malformed (bad encoding, traversal, missing fields) rather than +// unmatched. type InvalidRequestError struct { Reason string } @@ -35,18 +34,21 @@ type RoutedRequest struct { // Router resolves incoming requests (method + path + headers) against // rendered accept file rules: rule matching, origin/pool resolution, -// target URL construction, and header/auth injection. It is shared by -// every relay transport (the gRPC tunnel today, the snyk-broker reflector -// as it migrates) so accept-file semantics have exactly one -// implementation. +// wildcard-origin retargeting, target URL construction, and header/auth +// injection. It is the authoritative implementation of accept-file +// semantics; the snyk-broker reflector keeps its own copy of the wildcard +// policy until it is rerouted through this one. type Router struct { rules []AcceptFileRuleWrapper pools *PoolManager logger *zap.Logger } -// NewRouter creates a Router over rendered accept file rules. -func NewRouter(rules []AcceptFileRuleWrapper, logger *zap.Logger) *Router { +// NewRouter creates a Router over rendered accept file rules. It resolves and +// parses every rule origin up front, so a malformed policy — a wildcard that +// authorizes too much, a port that cannot be dialed — fails here rather than +// once per request. +func NewRouter(rules []AcceptFileRuleWrapper, logger *zap.Logger) (*Router, error) { if logger == nil { logger = zap.NewNop() } @@ -55,17 +57,44 @@ func NewRouter(rules []AcceptFileRuleWrapper, logger *zap.Logger) *Router { pools: NewPoolManager(), logger: logger.Named("accept-router"), } + for _, rule := range rules { warnUnsupportedRule(rule.dict, rt.logger) + // Peek rather than rotate: validation must not consume a pool slot and + // shift which member the first request lands on. + origin, err := rt.resolveOrigin(rule, false) + if err != nil { + rt.logger.Warn("Accept file rule has an unusable origin; requests matching it "+ + "will fail to route", + zap.String("rulePath", rule.Path()), zap.Error(err)) + continue + } + // A malformed wildcard origin is the one thing that stops the agent. + // The snyk-broker path refuses it too — at render, or by panicking when + // the reflector is off — so no working deployment has one, and treating + // a bad family as permissive would authorize hosts nobody chose. + parsed, err := parseOrigin(origin) + if err != nil { + return nil, fmt.Errorf("accept file rule %q: %w", rule.Path(), err) + } + // Anything else about the origin is left to fail per request, the way + // it does on the snyk-broker path, rather than at startup. + if parsed.url.Scheme == "" || parsed.url.Host == "" { + rt.logger.Warn("Accept file rule has an origin with no scheme or host; "+ + "requests matching it will fail to route", + zap.String("rulePath", rule.Path()), zap.String("origin", origin)) + } } - return rt + return rt, nil } -// Route resolves a request to a RoutedRequest. rawPath may carry a query -// string and percent-encoded segments; rules match on the decoded path -// without the query, and the encoded form is preserved on the resolved -// URL. Returns ErrNoRoute when no rule matches and *InvalidRequestError -// for malformed input. +// Route resolves a request to a RoutedRequest. rawPath may carry a fragment, a +// query string and percent-encoded segments; rules match on the decoded path +// alone, and the encoded form is preserved on the resolved URL. +// +// Returns ErrNoRoute when no rule matches, *InvalidRequestError for malformed +// input, and ErrDestinationRejected when a matched rule does not authorize the +// destination the request named. func (rt *Router) Route(method, rawPath string, headers map[string]string) (*RoutedRequest, error) { if method == "" || rawPath == "" { return nil, &InvalidRequestError{Reason: "missing method or path"} @@ -91,6 +120,11 @@ func (rt *Router) Route(method, rawPath string, headers map[string]string) (*Rou return nil, &InvalidRequestError{Reason: "path is not normalized"} } + // Taken before matching: internal routing metadata must not survive on any + // path — forwarded, logged or rejected — and must not be able to select a + // rule either. + requestedTargets, headers := takeTargetHosts(headers) + rule := MatchRule(rt.rules, method, decodedPath, headers) if rule == nil { return nil, ErrNoRoute @@ -110,8 +144,33 @@ func (rt *Router) Route(method, rawPath string, headers map[string]string) (*Rou } } - origin := rt.pools.ResolvePoolVars(rule.Origin()) - targetURL, err := buildTargetURL(origin, pathOnly, decodedPath, query) + origin, err := rt.resolveOrigin(*rule, true) + if err != nil { + return nil, err + } + parsed, err := parseOrigin(origin) + if err != nil { + return nil, fmt.Errorf("failed to resolve origin: %w", err) + } + + // A tunnel call carries no per-request routing once upgraded, so a family + // has no authority to upgrade against. + if parsed.isWildcard() && isWebSocketUpgrade(headers) { + rt.logger.Error("WebSocket upgrade is not supported for a wildcard origin", + zap.String("origin", origin)) + return nil, fmt.Errorf("%w: wildcard origin cannot carry a WebSocket upgrade", ErrDestinationRejected) + } + + // The reason is safe to log; the requested value is not. + targetHost, err := parsed.resolveTargetHost(requestedTargets) + if err != nil { + rt.logger.Error("Rejected destination", + zap.String("origin", origin), + zap.Error(err)) + return nil, err + } + + targetURL, err := buildTargetURL(parsed.url, targetHost, pathOnly, decodedPath, query) if err != nil { return nil, fmt.Errorf("failed to build target URL: %w", err) } @@ -159,42 +218,50 @@ func (rt *Router) Route(method, rawPath string, headers map[string]string) (*Rou }, nil } -// applyAuth sets the Authorization header from the rule's auth block. -func applyAuth(header http.Header, auth *AcceptFileRuleAuth) { - if auth == nil { - return - } - switch strings.ToLower(auth.Scheme) { - case "bearer", "token": - token := os.ExpandEnv(auth.Token) - header.Set("Authorization", fmt.Sprintf("Bearer %s", token)) - case "basic": - username := os.ExpandEnv(auth.Username) - password := os.ExpandEnv(auth.Password) - header.Set("Authorization", "Basic "+base64.StdEncoding.EncodeToString([]byte(username+":"+password))) - default: - // Custom scheme: set as Authorization header. - token := os.ExpandEnv(auth.Token) - header.Set("Authorization", fmt.Sprintf("%s %s", auth.Scheme, token)) +// resolveOrigin turns a rule's raw origin into a concrete URL string, rotating +// any ${VAR} that names a pool. advance is false for validation, which must not +// consume a slot. +func (rt *Router) resolveOrigin(rule AcceptFileRuleWrapper, advance bool) (string, error) { + raw := rule.RawOrigin() + if raw == "" { + return "", fmt.Errorf("rule has no origin") } + return defaultScheme(rt.pools.resolveVars(raw, advance)) } -// buildTargetURL joins the rule's resolved origin with the request path -// and query, preserving percent-encoded characters (e.g. %2F in GitLab -// project IDs) on the wire. -func buildTargetURL(origin, escapedPath, decodedPath, query string) (*url.URL, error) { - parsed, err := url.Parse(origin) +// defaultScheme fills in https for an origin written without one, which the +// accept-file format allows (${GITHUB:github.com}). +func defaultScheme(origin string) (string, error) { + asURL, err := url.Parse(origin) if err != nil { - return nil, err + return "", fmt.Errorf("invalid origin %q: %w", origin, err) + } + if asURL.Scheme != "" { + return origin, nil } - if parsed.Scheme == "" || parsed.Host == "" { - return nil, fmt.Errorf("origin %q has no scheme or host", origin) + asURL.Scheme = "https" + return asURL.String(), nil +} + +// buildTargetURL joins the resolved origin with the request path and query, +// preserving percent-encoded characters (e.g. %2F in GitLab project IDs) on the +// wire. targetHost, when set, replaces the origin's host — the origin keeps +// deciding the port. +func buildTargetURL(declared *url.URL, targetHost, escapedPath, decodedPath, query string) (*url.URL, error) { + if declared.Scheme == "" || declared.Host == "" { + return nil, fmt.Errorf("origin %q has no scheme or host", declared.String()) } - joinEscaped := joinPath(parsed.EscapedPath(), escapedPath) - joinDecoded := joinPath(parsed.Path, decodedPath) + joinEscaped := joinPath(declared.EscapedPath(), escapedPath) + joinDecoded := joinPath(declared.Path, decodedPath) - u := *parsed + u := *declared + if targetHost != "" { + if port := declared.Port(); port != "" { + targetHost = targetHost + ":" + port + } + u.Host = targetHost + } u.Path = joinDecoded if joinEscaped != joinDecoded { u.RawPath = joinEscaped diff --git a/agent/server/snykbroker/acceptfile/router_auth_test.go b/agent/server/snykbroker/acceptfile/router_auth_test.go new file mode 100644 index 0000000..ee9c774 --- /dev/null +++ b/agent/server/snykbroker/acceptfile/router_auth_test.go @@ -0,0 +1,153 @@ +package acceptfile + +import ( + "encoding/base64" + "fmt" + "testing" + + "github.com/stretchr/testify/require" +) + +// Auth parity with snyk-broker's lib/common/utils/auth-header.ts. Axon accept +// files ship only bearer and basic today, but a customer-authored file may use +// any of these and "drop-in replacement" has to mean it. + +func authRules(t *testing.T, authJSON string) *Router { + t.Helper() + t.Setenv("UPSTREAM", "https://up.example") + return newTestRouter(t, fmt.Sprintf( + `{"private":[{"method":"any","path":"/*","origin":"${UPSTREAM}","auth":%s}]}`, authJSON)) +} + +func authHeaderFor(t *testing.T, authJSON string) string { + t.Helper() + routed, err := authRules(t, authJSON).Route("GET", "/x", nil) + require.NoError(t, err) + return routed.Header.Get("Authorization") +} + +func TestAuthBearer(t *testing.T) { + t.Setenv("TOK", "abc") + require.Equal(t, "Bearer abc", authHeaderFor(t, `{"scheme":"bearer","token":"${TOK}"}`)) +} + +// The broker emits "Token", not "Bearer". They are different schemes and some +// upstreams accept only one. +func TestAuthTokenSchemeEmitsToken(t *testing.T) { + t.Setenv("TOK", "abc") + require.Equal(t, "Token abc", authHeaderFor(t, `{"scheme":"token","token":"${TOK}"}`)) +} + +// "raw" is the escape hatch for an upstream whose header carries no scheme +// prefix at all. +func TestAuthRawSchemeEmitsTheTokenVerbatim(t *testing.T) { + t.Setenv("TOK", "abc-123") + require.Equal(t, "abc-123", authHeaderFor(t, `{"scheme":"raw","token":"${TOK}"}`)) +} + +func TestAuthBasicFromUsernameAndPassword(t *testing.T) { + t.Setenv("USER", "svc-user") + t.Setenv("PASS", "svc-pass") + want := "Basic " + base64.StdEncoding.EncodeToString([]byte("svc-user:svc-pass")) + require.Equal(t, want, + authHeaderFor(t, `{"scheme":"basic","username":"${USER}","password":"${PASS}"}`)) +} + +// A basic block carrying "token" holds the already-encoded user:pass pair (the +// shape Azure Repos uses). Encoding it a second time sends a broken credential, +// and dropping it sends an empty one. +func TestAuthBasicWithPreEncodedToken(t *testing.T) { + encoded := base64.StdEncoding.EncodeToString([]byte("user:pass")) + t.Setenv("PRE_ENCODED", encoded) + require.Equal(t, "Basic "+encoded, + authHeaderFor(t, `{"scheme":"basic","token":"${PRE_ENCODED}"}`)) +} + +func TestAuthBasicPrefersUsernamePasswordWhenBothPresent(t *testing.T) { + t.Setenv("USER", "u") + t.Setenv("PASS", "p") + want := "Basic " + base64.StdEncoding.EncodeToString([]byte("u:p")) + require.Equal(t, want, + authHeaderFor(t, `{"scheme":"basic","username":"${USER}","password":"${PASS}","token":"ignored"}`)) +} + +func TestAuthSchemeIsCaseInsensitive(t *testing.T) { + t.Setenv("TOK", "abc") + require.Equal(t, "Bearer abc", authHeaderFor(t, `{"scheme":"Bearer","token":"${TOK}"}`)) + require.Equal(t, "Token abc", authHeaderFor(t, `{"scheme":"TOKEN","token":"${TOK}"}`)) +} + +// An unrecognized scheme used to become " " — a credential in a +// shape no upstream asked for. snyk-broker sends no header at all for one, so +// that is what the Router does; the warning is covered in +// TestUnknownAuthSchemeWarnsAndSendsNoHeader. +func TestUnknownAuthSchemeSendsNoHeader(t *testing.T) { + t.Setenv("TOK", "abc") + require.Empty(t, authHeaderFor(t, `{"scheme":"digest","token":"${TOK}"}`)) +} + +// The rule owns the credential; a caller cannot substitute its own. +func TestRuleAuthOverridesCallerAuthorization(t *testing.T) { + t.Setenv("TOK", "rule-token") + routed, err := authRules(t, `{"scheme":"bearer","token":"${TOK}"}`). + Route("GET", "/x", map[string]string{"Authorization": "Bearer caller-token"}) + require.NoError(t, err) + require.Equal(t, "Bearer rule-token", routed.Header.Get("Authorization")) +} + +// Every scheme the table declares must actually produce a credential, and +// nothing outside the table may produce one. This is what keeps "supported" +// meaning the same thing in both places it is consulted: the warning at Router +// construction, and applyAuth on the request. +// +// Before authHeaderBuilders these were two lists — a set in supported.go and a +// switch in router.go — that could disagree without anything noticing. +func TestEveryDeclaredAuthSchemeBuildsACredential(t *testing.T) { + require.NotEmpty(t, authHeaderBuilders) + + authBlock := func(scheme authScheme) string { + return `{"scheme":"` + string(scheme) + + `","token":"${TOK}","username":"${USER}","password":"${PASS}"}` + } + + for scheme := range authHeaderBuilders { + t.Run(string(scheme), func(t *testing.T) { + require.True(t, isSupportedAuthScheme(string(scheme)), + "a declared scheme must report as supported") + require.Contains(t, supportedAuthSchemes(), string(scheme), + "a declared scheme must be named in the warning") + + // Which field a scheme reads is its own business — basic prefers + // username/password and ignores the token. What has to hold for all + // of them is that a credential goes out and that it tracks the + // configuration, so a builder cannot quietly drop the secret. + t.Setenv("TOK", "tok-one") + t.Setenv("USER", "user-one") + t.Setenv("PASS", "pass-one") + first := authHeaderFor(t, authBlock(scheme)) + require.NotEmpty(t, first, "a supported scheme has to send a credential") + + t.Setenv("TOK", "tok-two") + t.Setenv("USER", "user-two") + t.Setenv("PASS", "pass-two") + second := authHeaderFor(t, authBlock(scheme)) + require.NotEqual(t, first, second, + "the credential has to come from the configuration, not a constant") + }) + } +} + +func TestUndeclaredAuthSchemeIsNotSupported(t *testing.T) { + for _, scheme := range []string{"digest", "negotiate", "", "bearer-ish"} { + require.False(t, isSupportedAuthScheme(scheme), "scheme=%q", scheme) + require.NotContains(t, supportedAuthSchemes(), scheme) + } +} + +// The lookup is on the scheme as the accept file spells it, so casing in the +// file cannot change whether a credential is sent. +func TestAuthSchemeLookupIsCaseInsensitive(t *testing.T) { + for _, spelling := range []string{"bearer", "Bearer", "BEARER", "BeArEr"} { + require.True(t, isSupportedAuthScheme(spelling), "spelling=%q", spelling) + } +} diff --git a/agent/server/snykbroker/acceptfile/router_credential_test.go b/agent/server/snykbroker/acceptfile/router_credential_test.go index a8b456e..0c9d86b 100644 --- a/agent/server/snykbroker/acceptfile/router_credential_test.go +++ b/agent/server/snykbroker/acceptfile/router_credential_test.go @@ -3,9 +3,7 @@ package acceptfile import ( "testing" - "github.com/cortexapps/axon/config" "github.com/stretchr/testify/require" - "go.uber.org/zap" ) // A credential provider that fails has to refuse the request, not let its @@ -19,18 +17,14 @@ import ( // puts it in the default arm of grpctunnel's RouteError mapping — a 502, the // same status the reflector sends. func TestRouteRefusesWhenACredentialProviderFails(t *testing.T) { - cfg := config.AgentConfig{HttpServerPort: 8080, PluginDirs: []string{"."}} - af, err := NewAcceptFile([]byte(`{"private":[{ + router := newTestRouter(t, `{"private":[{ "method": "any", "path": "/*", "origin": "https://api.example", "headers": {"authorization": "${plugin:plugin_fail.sh}"} - }]}`), cfg, zap.NewNop()) - require.NoError(t, err) - - router := NewRouter(af.Wrapper().PrivateRules(), zap.NewNop()) + }]}`, ".") - _, err = router.Route("GET", "/x", nil) + _, err := router.Route("GET", "/x", nil) require.Error(t, err, "a failing credential provider must fail the request") require.Contains(t, err.Error(), "credential provider failed") require.NotErrorIs(t, err, ErrNoRoute, "the rule matched; it was the credential that failed") @@ -39,16 +33,12 @@ func TestRouteRefusesWhenACredentialProviderFails(t *testing.T) { // The companion case: a provider that succeeds still reaches the upstream, so // the check above is refusing on the failure rather than on having a plugin. func TestRouteCarriesAResolvedPluginCredential(t *testing.T) { - cfg := config.AgentConfig{HttpServerPort: 8080, PluginDirs: []string{"."}} - af, err := NewAcceptFile([]byte(`{"private":[{ + router := newTestRouter(t, `{"private":[{ "method": "any", "path": "/*", "origin": "https://api.example", "headers": {"x-plugin-output": "${plugin:plugin.sh}"} - }]}`), cfg, zap.NewNop()) - require.NoError(t, err) - - router := NewRouter(af.Wrapper().PrivateRules(), zap.NewNop()) + }]}`, ".") req, err := router.Route("GET", "/x", nil) require.NoError(t, err) diff --git a/agent/server/snykbroker/acceptfile/router_pool_test.go b/agent/server/snykbroker/acceptfile/router_pool_test.go new file mode 100644 index 0000000..7188125 --- /dev/null +++ b/agent/server/snykbroker/acceptfile/router_pool_test.go @@ -0,0 +1,120 @@ +package acceptfile + +import ( + "sync" + "testing" + + "github.com/stretchr/testify/require" +) + +// Pool rotation has to happen where the origin is resolved for a request. +// Resolving ${VAR} against the environment first turns a pool-only variable +// into the empty string, and the PoolManager then has nothing left to rotate — +// so every request fails while pool_test.go stays green. + +func TestRouterRotatesPoolOrigins(t *testing.T) { + t.Setenv("POOLED_API_POOL", "https://a.example.com,https://b.example.com,https://c.example.com") + + rt := newTestRouter(t, `{"private":[{"method":"any","path":"/*","origin":"${POOLED_API}"}]}`) + + want := []string{ + "https://a.example.com/x", "https://b.example.com/x", "https://c.example.com/x", + "https://a.example.com/x", "https://b.example.com/x", "https://c.example.com/x", + } + for i, w := range want { + routed, err := rt.Route("GET", "/x", nil) + require.NoError(t, err, "request %d", i) + require.Equal(t, w, routed.URL.String(), "request %d", i) + } +} + +func TestRouterPoolTrimsWhitespaceAndEmptyEntries(t *testing.T) { + t.Setenv("POOLED_API_POOL", " https://a.example.com , ,https://b.example.com ") + + rt := newTestRouter(t, `{"private":[{"method":"any","path":"/*","origin":"${POOLED_API}"}]}`) + + for _, w := range []string{"https://a.example.com/x", "https://b.example.com/x", "https://a.example.com/x"} { + routed, err := rt.Route("GET", "/x", nil) + require.NoError(t, err) + require.Equal(t, w, routed.URL.String()) + } +} + +// A pool of one is a plain origin. +func TestRouterPoolOfOne(t *testing.T) { + t.Setenv("POOLED_API_POOL", "https://only.example.com") + + rt := newTestRouter(t, `{"private":[{"method":"any","path":"/*","origin":"${POOLED_API}"}]}`) + for i := 0; i < 3; i++ { + routed, err := rt.Route("GET", "/x", nil) + require.NoError(t, err) + require.Equal(t, "https://only.example.com/x", routed.URL.String()) + } +} + +// With no pool set the plain environment variable still wins. +func TestRouterFallsBackToPlainEnvVar(t *testing.T) { + t.Setenv("SINGLE_API", "https://api.example.com") + + rt := newTestRouter(t, `{"private":[{"method":"any","path":"/*","origin":"${SINGLE_API}"}]}`) + routed, err := rt.Route("GET", "/x", nil) + require.NoError(t, err) + require.Equal(t, "https://api.example.com/x", routed.URL.String()) +} + +// A pool beats the plain variable when both are set, matching the broker. +func TestRouterPoolBeatsPlainEnvVar(t *testing.T) { + t.Setenv("BOTH_API", "https://plain.example.com") + t.Setenv("BOTH_API_POOL", "https://pooled.example.com") + + rt := newTestRouter(t, `{"private":[{"method":"any","path":"/*","origin":"${BOTH_API}"}]}`) + routed, err := rt.Route("GET", "/x", nil) + require.NoError(t, err) + require.Equal(t, "https://pooled.example.com/x", routed.URL.String()) +} + +// Every concurrent request must land on a real pool member; the counter is the +// only shared state on the hot path. +func TestRouterPoolIsSafeUnderConcurrency(t *testing.T) { + t.Setenv("POOLED_API_POOL", "https://a.example.com,https://b.example.com") + + rt := newTestRouter(t, `{"private":[{"method":"any","path":"/*","origin":"${POOLED_API}"}]}`) + + const n = 200 + var wg sync.WaitGroup + var mu sync.Mutex + counts := map[string]int{} + + for i := 0; i < n; i++ { + wg.Add(1) + go func() { + defer wg.Done() + routed, err := rt.Route("GET", "/x", nil) + if err != nil { + return + } + mu.Lock() + counts[routed.URL.Host]++ + mu.Unlock() + }() + } + wg.Wait() + + require.Equal(t, n, counts["a.example.com"]+counts["b.example.com"], + "every request must resolve to a pool member") + require.Equal(t, n/2, counts["a.example.com"], "rotation must stay even") + require.Equal(t, n/2, counts["b.example.com"], "rotation must stay even") +} + +// A pool-only variable satisfies the accept file's required-variable check +// (varIsSet accepts VAR_POOL), so it must also route. +func TestPoolOnlyVariableSatisfiesLoadValidation(t *testing.T) { + t.Setenv("DECLARED_API_POOL", "https://a.example.com") + + rt := newTestRouter(t, `{"$vars":["${DECLARED_API}"], + "private":[{"method":"any","path":"/*","origin":"${DECLARED_API}"}]}`) + + routed, err := rt.Route("GET", "/x", nil) + require.NoError(t, err) + require.Equal(t, "https://a.example.com/x", routed.URL.String()) +} diff --git a/agent/server/snykbroker/acceptfile/router_test.go b/agent/server/snykbroker/acceptfile/router_test.go index a437d5a..b18e55d 100644 --- a/agent/server/snykbroker/acceptfile/router_test.go +++ b/agent/server/snykbroker/acceptfile/router_test.go @@ -25,7 +25,9 @@ func TestBuildTargetURL(t *testing.T) { t.Run(tt.origin+tt.path, func(t *testing.T) { decoded, err := urlPathUnescape(tt.path) require.NoError(t, err) - got, err := buildTargetURL(tt.origin, tt.path, decoded, tt.query) + parsed, err := parseOrigin(tt.origin) + require.NoError(t, err) + got, err := buildTargetURL(parsed.url, "", tt.path, decoded, tt.query) require.NoError(t, err) assert.Equal(t, tt.want, got.String()) }) @@ -33,9 +35,10 @@ func TestBuildTargetURL(t *testing.T) { } func TestRouter_NoRouteAndInvalid(t *testing.T) { - router := NewRouter(nil, nil) + router, err := NewRouter(nil, nil) + require.NoError(t, err) - _, err := router.Route("GET", "/nope", nil) + _, err = router.Route("GET", "/nope", nil) assert.ErrorIs(t, err, ErrNoRoute) _, err = router.Route("", "/nope", nil) diff --git a/agent/server/snykbroker/acceptfile/router_wildcard_test.go b/agent/server/snykbroker/acceptfile/router_wildcard_test.go new file mode 100644 index 0000000..05233f1 --- /dev/null +++ b/agent/server/snykbroker/acceptfile/router_wildcard_test.go @@ -0,0 +1,305 @@ +package acceptfile + +import ( + "fmt" + "os" + "path/filepath" + "sync" + "testing" + + "github.com/stretchr/testify/require" +) + +func wildcardRules(origin string) string { + return fmt.Sprintf(`{"private":[{"method":"any","path":"/*","origin":%q}]}`, origin) +} + +// --------------------------------------------------------------------------- +// Construction: a malformed policy fails at startup, not per request. +// --------------------------------------------------------------------------- + +func TestRouterRejectsMalformedWildcardOriginAtConstruction(t *testing.T) { + for name, origin := range map[string]string{ + "plaintext": "http://*.api.example.net", + "bare wildcard": "https://*", + "non-leftmost": "https://foo.*.api.example.net", + "public suffix": "https://*.com", + "port zero": "https://*.api.example.net:0", + } { + t.Run(name, func(t *testing.T) { + _, err := newTestRouterErr(t, wildcardRules(origin)) + require.Error(t, err, "origin=%q must be refused before any request", origin) + }) + } +} + +func TestRouterAcceptsWellFormedWildcardOriginAtConstruction(t *testing.T) { + _, err := newTestRouterErr(t, wildcardRules("https://*.axon.example.com")) + require.NoError(t, err) +} + +// --------------------------------------------------------------------------- +// Wildcard origins: the authority arrives per request and is checked first. +// --------------------------------------------------------------------------- + +func TestRouterWildcardRetargetsAcrossHosts(t *testing.T) { + rt := newTestRouter(t, wildcardRules("https://*.axon.example.com")) + + for _, host := range []string{"a.axon.example.com", "b.axon.example.com"} { + routed, err := rt.Route("GET", "/v1/things", map[string]string{HeaderTargetHost: host}) + require.NoError(t, err, "host=%q", host) + require.Equal(t, "https://"+host+"/v1/things", routed.URL.String()) + require.Empty(t, routed.Header.Values(HeaderTargetHost), + "routing metadata must not survive the hop") + } +} + +// The header names a host; the origin keeps deciding the port. +func TestRouterWildcardOriginPortReachesTheURL(t *testing.T) { + rt := newTestRouter(t, wildcardRules("https://*.something.com.internal:8443")) + + routed, err := rt.Route("GET", "/v1/things", map[string]string{ + HeaderTargetHost: "alpha.something.com.internal", + }) + require.NoError(t, err) + require.Equal(t, "alpha.something.com.internal:8443", routed.URL.Host) +} + +// A wildcard origin never falls back to a declared host: there isn't one. +func TestRouterWildcardWithoutTargetHostIsRejected(t *testing.T) { + rt := newTestRouter(t, wildcardRules("https://*.axon.example.com")) + + _, err := rt.Route("GET", "/v1/things", nil) + require.ErrorIs(t, err, ErrDestinationRejected) +} + +func TestRouterWildcardOutsidePolicyIsRejected(t *testing.T) { + rt := newTestRouter(t, wildcardRules("https://*.axon.example.com")) + + _, err := rt.Route("GET", "/v1/things", map[string]string{ + HeaderTargetHost: "evil.example.com", + }) + require.ErrorIs(t, err, ErrDestinationRejected) +} + +// A tunnel carries no per-request routing, so a family has no authority to +// upgrade against. +func TestRouterWildcardRefusesWebSocketUpgrade(t *testing.T) { + rt := newTestRouter(t, wildcardRules("https://*.axon.example.com")) + + _, err := rt.Route("GET", "/socket", map[string]string{ + HeaderTargetHost: "a.axon.example.com", + "Connection": "Upgrade", + "Upgrade": "websocket", + }) + require.ErrorIs(t, err, ErrDestinationRejected) +} + +func TestRouterConcreteOriginAcceptsWebSocketUpgrade(t *testing.T) { + rt := newTestRouter(t, wildcardRules("https://a.axon.example.com")) + + routed, err := rt.Route("GET", "/socket", map[string]string{ + "Connection": "Upgrade", + "Upgrade": "websocket", + }) + require.NoError(t, err) + require.Equal(t, "https://a.axon.example.com/socket", routed.URL.String()) +} + +// --------------------------------------------------------------------------- +// Concrete origins: never route on the header, but must still police and strip +// it, or the value reaches a third-party upstream. +// --------------------------------------------------------------------------- + +func TestRouterConcreteOriginStripsTargetHostAndRejectsDisagreement(t *testing.T) { + rt := newTestRouter(t, wildcardRules("https://a.axon.example.com")) + + routed, err := rt.Route("GET", "/v1/things", map[string]string{ + HeaderTargetHost: "a.axon.example.com", + }) + require.NoError(t, err) + require.Equal(t, "https://a.axon.example.com/v1/things", routed.URL.String()) + require.Empty(t, routed.Header.Values(HeaderTargetHost)) + + // Agreeing in a different case is still agreement. + routed, err = rt.Route("GET", "/v1/things", map[string]string{ + HeaderTargetHost: "A.Axon.Example.COM", + }) + require.NoError(t, err) + require.Equal(t, "https://a.axon.example.com/v1/things", routed.URL.String()) + + _, err = rt.Route("GET", "/v1/things", map[string]string{ + HeaderTargetHost: "alpha.api.example.net", + }) + require.ErrorIs(t, err, ErrDestinationRejected) +} + +func TestRouterConcreteOriginWithoutTargetHostIsUntouched(t *testing.T) { + rt := newTestRouter(t, wildcardRules("https://a.axon.example.com")) + + routed, err := rt.Route("GET", "/v1/things", map[string]string{"x-caller": "kept"}) + require.NoError(t, err) + require.Equal(t, "https://a.axon.example.com/v1/things", routed.URL.String()) + require.Equal(t, "kept", routed.Header.Get("x-caller")) + require.Empty(t, routed.Header.Values(HeaderTargetHost)) +} + +// The tunnel hands the Router a header map, so a duplicate arrives as two keys +// that differ only in case. Fail closed rather than picking one. +func TestRouterDuplicateTargetHostIsRejected(t *testing.T) { + rt := newTestRouter(t, wildcardRules("https://*.axon.example.com")) + + _, err := rt.Route("GET", "/v1/things", map[string]string{ + "X-Cortex-Target-Host": "a.axon.example.com", + "x-cortex-target-host": "b.axon.example.com", + }) + require.ErrorIs(t, err, ErrDestinationRejected) + + // Even when both spellings agree: two values is a shape we do not accept. + _, err = rt.Route("GET", "/v1/things", map[string]string{ + "X-Cortex-Target-Host": "a.axon.example.com", + "x-cortex-target-host": "a.axon.example.com", + }) + require.ErrorIs(t, err, ErrDestinationRejected) +} + +// However the caller spells it, the value is policed and removed. +func TestRouterTargetHostIsValidatedInEverySpelling(t *testing.T) { + for _, spelling := range []string{ + "x-cortex-target-host", + "X-Cortex-Target-Host", + "X-CORTEX-TARGET-HOST", + "x-Cortex-Target-host", + } { + t.Run(spelling, func(t *testing.T) { + rt := newTestRouter(t, wildcardRules("https://*.axon.example.com")) + + _, err := rt.Route("GET", "/v1/things", map[string]string{spelling: "evil.example.com"}) + require.ErrorIs(t, err, ErrDestinationRejected, "spelling must not bypass the policy") + + routed, err := rt.Route("GET", "/v1/things", map[string]string{spelling: "a.axon.example.com"}) + require.NoError(t, err) + require.Empty(t, routed.Header.Values(HeaderTargetHost)) + require.Empty(t, routed.Header.Values(spelling)) + }) + } +} + +func TestRouterWildcardConcurrentRetargets(t *testing.T) { + rt := newTestRouter(t, wildcardRules("https://*.axon.example.com")) + + const workers = 16 + const perWorker = 25 + var wg sync.WaitGroup + errs := make(chan error, workers*perWorker) + for w := 0; w < workers; w++ { + wg.Add(1) + go func(w int) { + defer wg.Done() + for i := 0; i < perWorker; i++ { + host := "a.axon.example.com" + if (w+i)%2 == 1 { + host = "b.axon.example.com" + } + routed, err := rt.Route("GET", "/v1/things", map[string]string{HeaderTargetHost: host}) + if err != nil { + errs <- fmt.Errorf("worker %d req %d: %w", w, i, err) + return + } + if got := routed.URL.Host; got != host { + errs <- fmt.Errorf("worker %d req %d: host=%q want %q", w, i, got, host) + return + } + } + }(w) + } + wg.Wait() + close(errs) + for err := range errs { + require.NoError(t, err) + } +} + +// --------------------------------------------------------------------------- +// The shipped Google template, end to end through the Router. +// --------------------------------------------------------------------------- + +func TestGoogleTemplateRoutesThroughTheWildcardFamily(t *testing.T) { + pluginDir := t.TempDir() + require.NoError(t, os.WriteFile(filepath.Join(pluginDir, "google-adc"), + []byte("#!/bin/sh\nprintf 'Bearer stub-token'\n"), 0700)) + + content, err := os.ReadFile(filepath.Join("..", "accept_files", "accept.google.json")) + require.NoError(t, err) + + rt := newTestRouter(t, string(content), pluginDir) + + routed, err := rt.Route("GET", "/storage/v1/b/my-bucket", map[string]string{ + HeaderTargetHost: "storage.googleapis.com", + }) + require.NoError(t, err) + require.Equal(t, "https://storage.googleapis.com/storage/v1/b/my-bucket", routed.URL.String()) + require.Equal(t, "Bearer stub-token", routed.Header.Get("authorization")) + require.Empty(t, routed.Header.Values(HeaderTargetHost)) + + // A host outside the family is refused even though the rule path matches. + _, err = rt.Route("GET", "/storage/v1/b/my-bucket", map[string]string{ + HeaderTargetHost: "evil.example.com", + }) + require.ErrorIs(t, err, ErrDestinationRejected) + + // And the family is not a free pass to any depth under it. + _, err = rt.Route("GET", "/storage/v1/b", map[string]string{ + HeaderTargetHost: "a.b.googleapis.com", + }) + require.ErrorIs(t, err, ErrDestinationRejected) +} + +// Routing metadata is taken off the request before rules are consulted, so it +// cannot be used to reach a rule the caller was not meant to reach. +func TestRouterTargetHostCannotSelectARule(t *testing.T) { + rt := newTestRouter(t, `{"private":[ + {"method":"any","path":"/*","origin":"https://gated.example.com", + "valid":[{"header":"x-cortex-target-host","values":["alpha.axon.example.com"]}]}, + {"method":"any","path":"/*","origin":"https://*.axon.example.com"} + ]}`) + + routed, err := rt.Route("GET", "/v1/things", map[string]string{ + HeaderTargetHost: "alpha.axon.example.com", + }) + require.NoError(t, err) + require.Equal(t, "https://alpha.axon.example.com/v1/things", routed.URL.String(), + "the gated rule must not be reachable through routing metadata") +} + +// An origin that resolves to nothing dialable warns at construction and fails +// the requests that match it, rather than stopping the agent. The snyk-broker +// path behaves the same way — it registers the rule and fails per request — so +// a deployment carrying one still starts on the tunnel. +func TestRouterWarnsOnUndialableOriginAndFailsPerRequest(t *testing.T) { + // A scheme-less origin carrying a port parses as an opaque URL with no + // host, which nothing can dial. + t.Setenv("NOT_A_URL", "github.com:8080") + rt, err := newTestRouterErr(t, `{"private":[ + {"method":"any","path":"/*","origin":"${NOT_A_URL}"}]}`) + require.NoError(t, err, "a bad origin must not stop the agent") + + _, err = rt.Route("GET", "/x", nil) + require.Error(t, err, "but the request it would have carried has to fail") + + // A rule with no origin at all is the same story. + rt, err = newTestRouterErr(t, `{"private":[{"method":"any","path":"/*"}]}`) + require.NoError(t, err) + _, err = rt.Route("GET", "/x", nil) + require.Error(t, err) + + // So is an origin that will not parse at all. Only a malformed *wildcard* + // stops the agent; a bad concrete origin is caught before parseOrigin sees + // it and left to fail per request. + t.Setenv("UNPARSEABLE", "http://%zz") + rt, err = newTestRouterErr(t, `{"private":[ + {"method":"any","path":"/*","origin":"${UNPARSEABLE}"}]}`) + require.NoError(t, err) + _, err = rt.Route("GET", "/x", nil) + require.Error(t, err) +} diff --git a/agent/server/snykbroker/acceptfile/supported.go b/agent/server/snykbroker/acceptfile/supported.go index 0735e6f..e0c27b3 100644 --- a/agent/server/snykbroker/acceptfile/supported.go +++ b/agent/server/snykbroker/acceptfile/supported.go @@ -1,6 +1,8 @@ package acceptfile import ( + "strings" + "go.uber.org/zap" ) @@ -12,9 +14,16 @@ import ( // an operator relying on it finds out from the log rather than from an agent // that will not boot, and can stay on snyk-broker until we implement it. // -// What ends up here are constructs snyk-broker honours that the Router does not -// implement — body and query "valid" filters, requiredCapabilities. Ignoring -// one widens the rule, so the warning says exactly that. +// Two classes end up here: +// +// - constructs snyk-broker honours that the Router does not yet implement — +// body and query "valid" filters, requiredCapabilities. Ignoring one widens +// the rule, so the warning says exactly that. +// - constructs snyk-broker itself effectively ignores — an unrecognized auth +// scheme, where authHeader() returns undefined and no Authorization header +// is sent at all. Ignoring it matches the broker; the warning is so nobody +// assumes a credential went out. +// // The one thing still refused is a malformed wildcard origin, and that is not a // migration risk: the snyk-broker path already refuses it too, at render // (ErrWildcardOriginRequiresTLSVerification, the invalid-origin error) or by @@ -60,6 +69,17 @@ func warnUnsupportedRule(rule map[string]any, logger *zap.Logger) int { "snyk-broker would reject a request that did not meet them.") } + if auth, ok := rule["auth"].(map[string]any); ok { + if scheme, _ := auth["scheme"].(string); !isSupportedAuthScheme(scheme) { + warnings++ + log.Warn( + "Ignoring an unrecognized auth scheme on an accept file rule: no Authorization "+ + "header will be sent, which is what snyk-broker does with it too.", + zap.String("scheme", scheme), + zap.String("supported", strings.Join(supportedAuthSchemes(), ", "))) + } + } + validEntries, ok := rule["valid"].([]any) if !ok { return warnings diff --git a/agent/server/snykbroker/acceptfile/unsupported_test.go b/agent/server/snykbroker/acceptfile/unsupported_test.go index 9752185..c1b2050 100644 --- a/agent/server/snykbroker/acceptfile/unsupported_test.go +++ b/agent/server/snykbroker/acceptfile/unsupported_test.go @@ -20,11 +20,9 @@ import ( // that switch has to be transparent: a file the broker accepts must still // start. Anything the Router cannot carry is warned about and ignored. // -// The refusal that used to be here is gone, and the warnings live at Router -// construction rather than at parse: parsing is shared with the snyk-broker -// path, where the Node broker honours these constructs, so warning at parse -// would tell an operator their working rule is being dropped when it is not. -// TestSnykBrokerConstructsStillParse pins that boundary. +// The one exception is a malformed wildcard origin, covered in +// router_wildcard_test.go — the snyk-broker path refuses that too, so no +// working deployment has one. // loadWithLogs parses an accept file with a logger the test can inspect. func loadWithLogs(t *testing.T, content string) (*AcceptFile, []observer.LoggedEntry, error) { @@ -35,8 +33,8 @@ func loadWithLogs(t *testing.T, content string) (*AcceptFile, []observer.LoggedE return af, logs.All(), err } -// routerWithLogs builds the Router, which is where a construct it cannot carry -// gets warned about. +// routerWithLogs builds the tunnel Router, which is where a construct it cannot +// carry gets warned about. func routerWithLogs(t *testing.T, content string) (*Router, []observer.LoggedEntry) { t.Helper() core, logs := observer.New(zapcore.WarnLevel) @@ -55,7 +53,9 @@ func routerWithLogs(t *testing.T, content string) (*Router, []observer.LoggedEnt rules = append(rules, r) } } - return NewRouter(rules, zap.New(core)), logs.All() + rt, err := NewRouter(rules, zap.New(core)) + require.NoError(t, err, "an unsupported construct must not stop the Router") + return rt, logs.All() } // requireWarns asserts the file loads, the Router builds, and a warning names @@ -173,6 +173,25 @@ func TestRequiredCapabilitiesWarnsAndIsIgnored(t *testing.T) { "requiredCapabilities":["post-streams"]}]}`, "requiredCapabilities") } +// An unrecognized scheme sends no Authorization header — which is exactly what +// snyk-broker's authHeader() does with it — but nobody should have to guess +// that a credential silently went missing. +func TestUnknownAuthSchemeWarnsAndSendsNoHeader(t *testing.T) { + t.Setenv("TOK", "abc") + logs := requireWarns(t, `{"private":[{ + "method":"any","path":"/*","origin":"https://up.example", + "auth":{"scheme":"digest","token":"${TOK}"}}]}`, "digest") + require.NotEmpty(t, logs) + + rt, _ := routerWithLogs(t, `{"private":[{ + "method":"any","path":"/*","origin":"https://up.example", + "auth":{"scheme":"digest","token":"${TOK}"}}]}`) + routed, err := rt.Route("GET", "/x", nil) + require.NoError(t, err) + require.Empty(t, routed.Header.Get("Authorization"), + "an unrecognized scheme sends no credential, as in snyk-broker") +} + // The tunnel streams every body, so "stream": true asks for what it already // does — no warning, nothing to ignore. func TestStreamFieldIsAcceptedSilently(t *testing.T) { @@ -248,13 +267,15 @@ func TestNoAcceptFileConstructStopsTheAgent(t *testing.T) { "valid":[{"queryParam":"proxyMe","values":["please"]}]}]}`, "required capabilities": `{"private":[{"method":"GET","path":"/*","origin":"https://up.example", "requiredCapabilities":["post-streams"]}]}`, + "unknown auth scheme": `{"private":[{"method":"GET","path":"/*","origin":"https://up.example", + "auth":{"scheme":"digest","token":"t"}}]}`, "inbound rules": `{"private":[{"method":"any","path":"/*","origin":"https://up.example"}], "public":[{"method":"POST","path":"/webhook/github"}]}`, "stream": `{"private":[{"method":"GET","path":"/*","origin":"https://up.example","stream":true}]}`, "unknown rule field": `{"private":[{"method":"GET","path":"/*","origin":"https://up.example","someFutureField":{"a":1}}]}`, "comment key": `{"private":[{"//":"note","method":"GET","path":"/*","origin":"https://up.example"}]}`, "everything at once": `{"private":[{"method":"POST","path":"/*","origin":"https://up.example", - "requiredCapabilities":["x"], + "requiredCapabilities":["x"],"auth":{"scheme":"digest","token":"t"}, "valid":[{"path":"a.b","value":"c"},{"queryParam":"q","values":["v"]}]}], "public":[{"method":"POST","path":"/hook"}]}`, } { @@ -267,26 +288,6 @@ func TestNoAcceptFileConstructStopsTheAgent(t *testing.T) { } } -// The snyk-broker path keeps working. Every construct the Router warns about is -// one the Node broker implements, so parsing has to stay silent about it: an -// operator on snyk-broker must not be told their working rule is being dropped. -func TestSnykBrokerConstructsStillParse(t *testing.T) { - for name, content := range map[string]string{ - "body filter": `{"private":[{"method":"POST","path":"/*","origin":"https://up.example", - "valid":[{"path":"proxy.*","value":"please"}]}]}`, - "query filter": `{"private":[{"method":"GET","path":"/*","origin":"https://up.example", - "valid":[{"queryParam":"proxyMe","values":["please"]}]}]}`, - "required capabilities": `{"private":[{"method":"GET","path":"/*","origin":"https://up.example", - "requiredCapabilities":["post-streams"]}]}`, - } { - t.Run(name, func(t *testing.T) { - _, logs, err := loadWithLogs(t, content) - require.NoError(t, err, "the Node broker honours this; parsing must not refuse it") - require.Empty(t, logs, "nor tell a snyk-broker operator it is being ignored") - }) - } -} - func TestUnknownRuleKeysAreAcceptedSilently(t *testing.T) { _, logs := routerWithLogs(t, `{"private":[{ "//":"the catch-all API rule", @@ -308,9 +309,7 @@ func TestEveryShippedAcceptFileLoads(t *testing.T) { } // Accept files the repo hands to the agent — the shipped templates and the -// fixtures the docker E2E suites run with. Without the fixtures here, a -// construct this package starts refusing surfaces only as a container that -// will not start, several CI minutes later. +// fixtures the docker E2E suites run with. func acceptFilesUnderTest(t *testing.T) []string { t.Helper() files := builtinAcceptFiles(t) diff --git a/agent/test/conformance/README.md b/agent/test/conformance/README.md index 6ca82ba..894eb06 100644 --- a/agent/test/conformance/README.md +++ b/agent/test/conformance/README.md @@ -22,29 +22,58 @@ Each fixture is one JSON file: "expect": { "matched": true, "url": "https://origin.example/x", - "headers": { "authorization": "Bearer t" } + "headers": { "authorization": "Bearer t" }, + "absentHeaders": ["x-cortex-target-host"] } } ] } ``` -- `expect.matched: false` asserts the request is rejected (no rule). +- `expect.matched: false` asserts the request is rejected. +- `expect.code` is the status a rejected request carries. It defaults to + `404` (no rule matched); `403` says a rule matched but did not + authorize the destination the request named; `400` says the request + was malformed before matching (bad encoding, directory traversal). - `expect.url` asserts the exact URL (path + query included) that would be sent to the upstream. - `expect.headers` asserts a **subset** of the outgoing request headers (case-insensitive names). +- `expect.absentHeaders` asserts headers that must NOT reach the + upstream — internal routing metadata, chiefly. Runners: 1. `agent/server/grpctunnel/conformance_test.go` runs every fixture through the Go path (accept-file render → Router.Route) on every test run. -2. The snyk-broker docker E2E harness (`agent/test/relay/`) is the second - runner: it plays the same cases through a live broker and asserts what - reaches a mock upstream. Divergence — including pre-existing quirks of - the Node broker — fails the build and forces a decision: match the - broker's behaviour, or document the exception here with a reason. +2. A second runner playing the same cases through a live broker + (`agent/test/relay/`) is **not built yet**. Until it is, divergence + from the Node broker is caught by review and by the fixtures below + rather than by the build, so a fixture that encodes broker parity + should say so in its case names. + +Deliberate divergences from snyk-broker, so they are not mistaken for +bugs: + +- `valid` header values are compared case-insensitively, and an empty + `values` array means "the header must be present". snyk-broker + compares case-sensitively and an empty array rejects everything. Both + differences are more permissive, so a request the broker routed still + routes. +- **Nothing in an accept file stops the agent.** Enabling the tunnel + switches deployments that run on snyk-broker today, and a file the + broker accepts has to still start. Constructs the Router cannot carry — + body and query `valid` filters, `requiredCapabilities`, unrecognized + auth schemes, inbound `public` rules — are warned about and ignored. + A warning that changes what a rule matches says so. + The one exception is a malformed wildcard origin, which the + snyk-broker path refuses too (at render, or by panicking when the + reflector is off), so no working deployment carries one. +- `${VAR}` in a `path` is a **segment placeholder, not a filter**: it + matches whatever the caller sent there and the configured value is + substituted into the outgoing URL. That is snyk-broker's behaviour, + quirks included. When adding an accept-file feature, add fixture cases FIRST — they define the semantics both transports must implement. diff --git a/agent/test/conformance/auth_schemes.json b/agent/test/conformance/auth_schemes.json new file mode 100644 index 0000000..0ecb10c --- /dev/null +++ b/agent/test/conformance/auth_schemes.json @@ -0,0 +1,108 @@ +{ + "name": "auth schemes (snyk-broker auth-header.ts parity)", + "env": { + "UPSTREAM": "https://upstream.example", + "API_TOKEN": "tok-123", + "PRE_ENCODED": "dXNlcjpwYXNz", + "PINNED_ORG": "acme" + }, + "acceptFile": { + "private": [ + { + "method": "GET", + "path": "/token/*", + "origin": "${UPSTREAM}", + "auth": { + "scheme": "token", + "token": "${API_TOKEN}" + } + }, + { + "method": "GET", + "path": "/raw/*", + "origin": "${UPSTREAM}", + "auth": { + "scheme": "raw", + "token": "${API_TOKEN}" + } + }, + { + "method": "GET", + "path": "/basic-token/*", + "origin": "${UPSTREAM}", + "auth": { + "scheme": "basic", + "token": "${PRE_ENCODED}" + } + }, + { + "method": "GET", + "path": "/repos/${PINNED_ORG}/*", + "origin": "${UPSTREAM}" + } + ] + }, + "cases": [ + { + "name": "the token scheme emits Token, not Bearer", + "request": { + "method": "GET", + "path": "/token/thing" + }, + "expect": { + "matched": true, + "headers": { + "authorization": "Token tok-123" + } + } + }, + { + "name": "the raw scheme emits the token with no prefix", + "request": { + "method": "GET", + "path": "/raw/thing" + }, + "expect": { + "matched": true, + "headers": { + "authorization": "tok-123" + } + } + }, + { + "name": "a basic block carrying a token holds the encoded pair already", + "request": { + "method": "GET", + "path": "/basic-token/thing" + }, + "expect": { + "matched": true, + "headers": { + "authorization": "Basic dXNlcjpwYXNz" + } + } + }, + { + "name": "${VAR} in a path is substituted into the outgoing URL", + "request": { + "method": "GET", + "path": "/repos/acme/widget" + }, + "expect": { + "matched": true, + "url": "https://upstream.example/repos/acme/widget" + } + }, + { + "name": "another segment matches and is rewritten, as snyk-broker does", + "request": { + "method": "GET", + "path": "/repos/evilcorp/widget" + }, + "expect": { + "matched": true, + "url": "https://upstream.example/repos/acme/widget" + } + } + ] +} diff --git a/agent/test/conformance/wildcard_origins.json b/agent/test/conformance/wildcard_origins.json new file mode 100644 index 0000000..58fa31e --- /dev/null +++ b/agent/test/conformance/wildcard_origins.json @@ -0,0 +1,104 @@ +{ + "name": "wildcard origins and per-request target hosts", + "env": { + "FAMILY": "https://*.axon.example.com", + "CONCRETE": "https://a.axon.example.com" + }, + "acceptFile": { + "private": [ + { "method": "any", "path": "/family/*", "origin": "${FAMILY}" }, + { "method": "any", "path": "/concrete/*", "origin": "${CONCRETE}" } + ] + }, + "cases": [ + { + "name": "target host inside the family is dialed", + "request": { + "method": "GET", + "path": "/family/v1/things", + "headers": { "x-cortex-target-host": "alpha.axon.example.com" } + }, + "expect": { + "matched": true, + "url": "https://alpha.axon.example.com/family/v1/things", + "absentHeaders": ["x-cortex-target-host"] + } + }, + { + "name": "a second host in the family retargets independently", + "request": { + "method": "GET", + "path": "/family/v1/things", + "headers": { "x-cortex-target-host": "beta.axon.example.com" } + }, + "expect": { "matched": true, "url": "https://beta.axon.example.com/family/v1/things" } + }, + { + "name": "the header name is case-insensitive", + "request": { + "method": "GET", + "path": "/family/v1/things", + "headers": { "X-Cortex-Target-Host": "alpha.axon.example.com" } + }, + "expect": { "matched": true, "url": "https://alpha.axon.example.com/family/v1/things" } + }, + { + "name": "a wildcard origin without a target host is rejected", + "request": { "method": "GET", "path": "/family/v1/things" }, + "expect": { "matched": false, "code": 403 } + }, + { + "name": "a target host outside the family is rejected", + "request": { + "method": "GET", + "path": "/family/v1/things", + "headers": { "x-cortex-target-host": "evil.example.com" } + }, + "expect": { "matched": false, "code": 403 } + }, + { + "name": "the wildcard covers exactly one label", + "request": { + "method": "GET", + "path": "/family/v1/things", + "headers": { "x-cortex-target-host": "a.b.axon.example.com" } + }, + "expect": { "matched": false, "code": 403 } + }, + { + "name": "a wildcard origin refuses a websocket upgrade", + "request": { + "method": "GET", + "path": "/family/socket", + "headers": { + "x-cortex-target-host": "alpha.axon.example.com", + "connection": "Upgrade", + "upgrade": "websocket" + } + }, + "expect": { "matched": false, "code": 403 } + }, + { + "name": "a concrete origin strips the header rather than forwarding it", + "request": { + "method": "GET", + "path": "/concrete/v1/things", + "headers": { "x-cortex-target-host": "a.axon.example.com" } + }, + "expect": { + "matched": true, + "url": "https://a.axon.example.com/concrete/v1/things", + "absentHeaders": ["x-cortex-target-host"] + } + }, + { + "name": "a concrete origin rejects a target host that disagrees with it", + "request": { + "method": "GET", + "path": "/concrete/v1/things", + "headers": { "x-cortex-target-host": "alpha.axon.example.com" } + }, + "expect": { "matched": false, "code": 403 } + } + ] +}