Never let an accept file stop the agent

agent/server/snykbroker/acceptfile/accept_file.go

@@ -52,6 +52,12 @@ func NewAcceptFile(content []byte, cfg config.AgentConfig, logger *zap.Logger) (
 	}
 
 	af.wrapper = newAcceptFileWrapper(processedContent, af)
+	// Deliberately no strict validation here. Parsing an accept file is shared
+	// with the snyk-broker path, where the Node broker honours constructs this
+	// package does not — refusing them here would break a deployment that
+	// works today. The Router warns about them when it is built instead, where
+	// they would otherwise be silently dropped.
+	warnIgnoredPublicRules(af.wrapper.dict, af.logger)
 	return af, nil
 }
 

agent/server/snykbroker/acceptfile/origin_contract_test.go

@@ -0,0 +1,119 @@
+package acceptfile
+
+import (
+	"testing"
+
+	"github.com/cortexapps/axon/config"
+	"github.com/stretchr/testify/require"
+	"go.uber.org/zap"
+)
+
+// 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.
+//
+// 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.
+func TestOriginContract(t *testing.T) {
+	cases := []struct {
+		name   string
+		origin string
+		env    map[string]string
+		want   string
+	}{
+		{
+			name:   "absolute origin is returned verbatim",
+			origin: "https://api.github.com",
+			want:   "https://api.github.com",
+		},
+		{
+			name:   "origin keeps its base path",
+			origin: "https://git.example/api/v3",
+			want:   "https://git.example/api/v3",
+		},
+		{
+			name:   "origin keeps its port",
+			origin: "http://localhost:9999",
+			want:   "http://localhost:9999",
+		},
+		{
+			name:   "plaintext scheme is not upgraded",
+			origin: "http://internal.example",
+			want:   "http://internal.example",
+		},
+		{
+			name:   "a scheme-less origin defaults to https",
+			origin: "github.com",
+			want:   "https://github.com",
+		},
+		{
+			name:   "a scheme-less default from ${VAR:default} defaults to https",
+			origin: "${GITHUB:github.com}",
+			want:   "https://github.com",
+		},
+		{
+			name:   "${VAR} expands from the environment",
+			origin: "${GITHUB_API}",
+			env:    map[string]string{"GITHUB_API": "https://ghe.example/api/v3"},
+			want:   "https://ghe.example/api/v3",
+		},
+		{
+			name:   "${VAR:default} prefers the environment when set",
+			origin: "${GITHUB:github.com}",
+			env:    map[string]string{"GITHUB": "https://ghe.example"},
+			want:   "https://ghe.example",
+		},
+		{
+			// The reflector greps Origin() for "*" to decide a rule needs a
+			// wildcard policy, so the star has to survive verbatim.
+			name:   "a wildcard family survives untouched",
+			origin: "https://*.googleapis.com",
+			want:   "https://*.googleapis.com",
+		},
+		{
+			name:   "a wildcard family keeps its port",
+			origin: "https://*.api.example.net:8443",
+			want:   "https://*.api.example.net:8443",
+		},
+		{
+			name:   "a wildcard family from a ${VAR:default}",
+			origin: "${GOOGLE_API:https://*.googleapis.com}",
+			want:   "https://*.googleapis.com",
+		},
+		{
+			name:   "credentials in the origin are preserved",
+			origin: "http://user:pass@localhost:9000",
+			want:   "http://user:pass@localhost:9000",
+		},
+	}
+
+	for _, tc := range cases {
+		t.Run(tc.name, func(t *testing.T) {
+			for k, v := range tc.env {
+				t.Setenv(k, v)
+			}
+			cfg := config.AgentConfig{HttpServerPort: 8080, PluginDirs: []string{}}
+			content := `{"private":[{"method":"any","path":"/*","origin":"` + tc.origin + `"}]}`
+			af, err := NewAcceptFile([]byte(content), cfg, zap.NewNop())
+			require.NoError(t, err)
+
+			rules := af.Wrapper().PrivateRules()
+			require.Len(t, rules, 1)
+			require.Equal(t, tc.want, rules[0].Origin())
+		})
+	}
+}
+
+// A rule with no origin reads as empty rather than panicking: the reflector
+// calls Origin() on every private rule before anything has vetted them.
+func TestOriginOfRuleWithoutOneIsEmpty(t *testing.T) {
+	cfg := config.AgentConfig{HttpServerPort: 8080, PluginDirs: []string{}}
+	af, err := NewAcceptFile([]byte(`{"private":[{"method":"any","path":"/*"}]}`), cfg, zap.NewNop())
+	require.NoError(t, err)
+
+	rules := af.Wrapper().PrivateRules()
+	require.Len(t, rules, 1)
+	require.Equal(t, "", rules[0].Origin())
+}

agent/server/snykbroker/acceptfile/router.go

@@ -50,11 +50,15 @@ func NewRouter(rules []AcceptFileRuleWrapper, logger *zap.Logger) *Router {
 	if logger == nil {
 		logger = zap.NewNop()
 	}
-	return &Router{
+	rt := &Router{
 		rules:  rules,
 		pools:  NewPoolManager(),
 		logger: logger.Named("accept-router"),
 	}
+	for _, rule := range rules {
+		warnUnsupportedRule(rule.dict, rt.logger)
+	}
+	return rt
 }
 
 // Route resolves a request to a RoutedRequest. rawPath may carry a query
@@ -92,7 +96,22 @@ func (rt *Router) Route(method, rawPath string, headers map[string]string) (*Rou
 
 	// Inject rule headers (overrides incoming).
 	if ruleHeaders := rule.Headers(); ruleHeaders != nil {
-		for k, v := range ruleHeaders.ToStringMap() {
+		// A credential provider that fails has to stop the request rather than
+		// let its placeholder travel upstream as the credential — the refusal
+		// then comes back as an authorization failure and names the wrong
+		// culprit, which is the thing #127 set out to stop. The reflector's
+		// serve() answers 502 here; returning an error lands in the default
+		// arm of grpctunnel's RouteError mapping, which is also a 502, so both
+		// transports refuse the same way.
+		resolved, err := ruleHeaders.ToStringMap()
+		if err != nil {
+			rt.logger.Error("Credential provider failed",
+				zap.String("rulePath", rule.Path()),
+				zap.Error(err),
+			)
+			return nil, fmt.Errorf("credential provider failed: %w", err)
+		}
+		for k, v := range resolved {
 			header.Set(k, v)
 		}
 	}

agent/server/snykbroker/acceptfile/router_credential_test.go

@@ -0,0 +1,56 @@
+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
+// placeholder travel upstream as the credential — the upstream then refuses on
+// authorization and the log names the wrong culprit. #127 made ResolverMap
+// report that failure instead of swallowing it, and taught the reflector's
+// serve() to answer 502. This pins the same behaviour on the tunnel path,
+// which reads the same ResolverMap through Router.Route.
+//
+// Route returns a plain error here rather than a classified one, which is what
+// 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":[{
+		"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)
+	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")
+}
+
+// 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":[{
+		"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)
+	require.NotEmpty(t, req.Header.Get("x-plugin-output"))
+}

agent/server/snykbroker/acceptfile/supported.go

@@ -0,0 +1,96 @@
+package acceptfile
+
+import (
+	"go.uber.org/zap"
+)
+
+// Nothing in an accept file stops the agent.
+//
+// Enabling the tunnel switches deployments that are running on snyk-broker
+// today, and that switch has to be transparent: a file the broker accepts must
+// still start. Anything the Router cannot carry is warned about and ignored, so
+// 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.
+// 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
+// panicking when the reflector is disabled. No working deployment has one, and
+// treating a bad family as permissive would authorize hosts nobody chose.
+
+// warnIgnoredPublicRules logs once for an accept file that declares inbound
+// rules, and reports how many it found.
+//
+// A "public" block describes webhook traffic the relay does not carry, so it
+// cannot widen what the agent will call outbound — and enough files carry one
+// copied from a snyk-broker config that refusing them would break working
+// deployments over a section that routes nothing.
+//
+// An empty block is silent: it is what Render itself emits, and warning about
+// it would train everyone to ignore the warning.
+func warnIgnoredPublicRules(dict map[string]any, logger *zap.Logger) int {
+	rules, ok := dict[RULES_PUBLIC].([]any)
+	if !ok || len(rules) == 0 {
+		return 0
+	}
+	logger.Warn(
+		"Ignoring inbound rules in the accept file: the relay carries no inbound traffic, "+
+			"so these route nothing. Support for the section will be removed — remove it from your accept file.",
+		zap.String("section", RULES_PUBLIC),
+		zap.Int("rules", len(rules)),
+	)
+	return len(rules)
+}
+
+// warnUnsupportedRule logs whatever in a rule the Router will not act on, and
+// reports how many warnings it emitted.
+func warnUnsupportedRule(rule map[string]any, logger *zap.Logger) int {
+	path, _ := rule["path"].(string)
+	log := logger.With(zap.String("rulePath", path))
+	warnings := 0
+
+	if _, present := rule["requiredCapabilities"]; present {
+		warnings++
+		log.Warn(
+			"Ignoring \"requiredCapabilities\" on an accept file rule: the relay negotiates no " +
+				"client capabilities, so the rule is allowed through unconditionally. " +
+				"snyk-broker would reject a request that did not meet them.")
+	}
+
+	validEntries, ok := rule["valid"].([]any)
+	if !ok {
+		return warnings
+	}
+	for _, entry := range validEntries {
+		entryDict, ok := entry.(map[string]any)
+		if !ok {
+			continue
+		}
+		if key := unsupportedValidKey(entryDict); key != "" {
+			warnings++
+			log.Warn(
+				"Ignoring a \"valid\" entry on an accept file rule: the relay does not inspect the "+
+					"request body or query string, so this narrowing is not applied and the rule "+
+					"matches more than it does under snyk-broker.",
+				zap.String("filter", key))
+		}
+	}
+	return warnings
+}
+
+// unsupportedValidKey names why a "valid" entry cannot be honoured, or "" when
+// it is a header requirement the matcher does apply.
+func unsupportedValidKey(entry map[string]any) string {
+	for _, key := range []string{"path", "regex", "queryParam"} {
+		if _, present := entry[key]; present {
+			return key
+		}
+	}
+	if header, _ := entry["header"].(string); header == "" {
+		return "no recognized key"
+	}
+	return ""
+}