diff --git a/docs/learn/services.mdx b/docs/learn/services.mdx index 2015a7b6..6d727f3a 100644 --- a/docs/learn/services.mdx +++ b/docs/learn/services.mdx @@ -290,7 +290,28 @@ Worked example — Slack with two credentials at the same host: - To override a wildcard-host rule for a specific subdomain, **add the exact-host rule** — it takes priority no matter what its path portion looks like. - To layer two credentials on the same host, give them **different path portions**; the more specific path will win. -- The matcher does not run regex, does not match on method/headers/query/body, and does not bound `*` at path segments. Patterns that tie under rule 2 (e.g. `slack.com/api/*/v2` and `slack.com/api/*/v3`) fall to declaration order. +- The matcher does not run regex, does not match on method/headers/query/body, and does not bound `*` at path segments. Patterns that tie under rule 2 (e.g. `slack.com/api/*/v2` and `slack.com/api/*/v3`) fall to declaration order. (A service's optional `methods` list is not a matcher dimension — it is checked after selection; see [Restricting HTTP methods](#restricting-http-methods).) + +## Restricting HTTP methods + +A service may declare an optional `methods` list. When present, the credential is attached only to requests using one of the listed HTTP methods; any other method is denied with a 403 (`method_not_allowed`) and the credential is not attached in any form — neither as auth headers nor through substitutions. + +```yaml +services: + - name: github-readonly + host: api.github.com + methods: [GET, HEAD] + auth: + type: bearer + token: GITHUB_TOKEN +``` + +- Omitting `methods` allows all methods. An explicitly empty list (`methods: []`) is rejected by validation. +- Supported values: `GET`, `HEAD`, `POST`, `PUT`, `PATCH`, `DELETE`, `OPTIONS`. Values are uppercased on ingest. +- `methods` is **not** part of matching. The matcher selects the single most-specific host/port/path winner exactly as described above, and the method check then runs on that winner — like a disabled service, a method-excluded winner is denied outright with no fall-through to a broader sibling. Making a whole host read-only therefore requires `methods` on every service whose pattern can win requests for that host. +- On a method-restricted service, Agent Vault also strips the well-known method-override surfaces before forwarding: the `X-HTTP-Method-Override`, `X-Method-Override`, and `X-HTTP-Method` headers and the `_method` query parameter. The `_method` form carried in a POST body is not inspected. + +Method restriction constrains plain HTTP request semantics only. A WebSocket handshake is a `GET`, so a `methods: [GET]` service still matches it, and `websocket`-scope substitutions write into frames that carry no method. ## Example diff --git a/internal/broker/broker.go b/internal/broker/broker.go index de2bd67f..188568e5 100644 --- a/internal/broker/broker.go +++ b/internal/broker/broker.go @@ -33,6 +33,7 @@ type Service struct { Path string `yaml:"path,omitempty" json:"path,omitempty"` Port *int `yaml:"port,omitempty" json:"-"` Enabled *bool `yaml:"enabled,omitempty" json:"enabled,omitempty"` + Methods []string `yaml:"methods,omitempty" json:"methods,omitempty"` Auth Auth `yaml:"auth" json:"auth"` Substitutions []Substitution `yaml:"substitutions,omitempty" json:"substitutions,omitempty"` } @@ -75,6 +76,57 @@ func (s *Service) IsEnabled() bool { return s.Enabled == nil || *s.Enabled } +// SupportedMethods lists the HTTP methods a service's Methods field may +// contain. +var SupportedMethods = []string{"GET", "HEAD", "POST", "PUT", "PATCH", "DELETE", "OPTIONS"} + +// NormalizeAndValidateMethods uppercases s.Methods in place, then checks +// each value against SupportedMethods and rejects duplicates. A nil +// Methods field means "all methods"; an explicitly-empty list is +// rejected so `methods: []` can't silently mean "allow everything". +func (s *Service) NormalizeAndValidateMethods() error { + if s.Methods == nil { + return nil + } + if len(s.Methods) == 0 { + return fmt.Errorf("methods: must not be an empty list (omit the field to allow all methods)") + } + allowed := make(map[string]bool, len(SupportedMethods)) + for _, m := range SupportedMethods { + allowed[m] = true + } + seen := make(map[string]bool, len(s.Methods)) + for i, m := range s.Methods { + u := strings.ToUpper(strings.TrimSpace(m)) + if !allowed[u] { + return fmt.Errorf("methods: unsupported method %q (supported: %s)", m, strings.Join(SupportedMethods, ", ")) + } + if seen[u] { + return fmt.Errorf("methods: duplicate method %q", u) + } + seen[u] = true + s.Methods[i] = u + } + return nil +} + +// AllowsMethod reports whether the service permits the given request +// method. A service with no Methods list permits all methods. The +// comparison is case-sensitive against the uppercase-normalized list, +// so a non-canonical-case request method fails closed rather than being +// coerced into a match. +func (s *Service) AllowsMethod(method string) bool { + if len(s.Methods) == 0 { + return true + } + for _, m := range s.Methods { + if m == method { + return true + } + } + return false +} + // Auth describes how credentials are attached for a broker service. // Each service must specify a Type and the fields relevant to that type. // @@ -373,6 +425,11 @@ func Validate(cfg *Config) error { if err := ValidatePort(s.Port); err != nil { return fmt.Errorf("service %d: %w", i, err) } + // Normalize through the slice element, not the range copy, so the + // uppercased methods persist. + if err := cfg.Services[i].NormalizeAndValidateMethods(); err != nil { + return fmt.Errorf("service %d: %w", i, err) + } if err := s.Auth.Validate(); err != nil { return fmt.Errorf("service %d: %w", i, err) } diff --git a/internal/broker/broker_methods_test.go b/internal/broker/broker_methods_test.go new file mode 100644 index 00000000..2153dee9 --- /dev/null +++ b/internal/broker/broker_methods_test.go @@ -0,0 +1,78 @@ +package broker + +import ( + "strings" + "testing" +) + +func methodsCfg(methods []string) *Config { + return &Config{ + Vault: "v1", + Services: []Service{{ + Name: "api", + Host: "api.example.com", + Methods: methods, + Auth: Auth{Type: "bearer", Token: "TOKEN"}, + }}, + } +} + +func TestValidate_MethodsNormalized(t *testing.T) { + cfg := methodsCfg([]string{"get", "Post"}) + if err := Validate(cfg); err != nil { + t.Fatalf("unexpected err: %v", err) + } + got := cfg.Services[0].Methods + if len(got) != 2 || got[0] != "GET" || got[1] != "POST" { + t.Fatalf("methods not normalized in place: %v", got) + } +} + +func TestValidate_MethodsRejectsUnsupported(t *testing.T) { + err := Validate(methodsCfg([]string{"GET", "FETCH"})) + if err == nil || !strings.Contains(err.Error(), "unsupported method") { + t.Fatalf("want unsupported-method error, got %v", err) + } +} + +func TestValidate_MethodsRejectsDuplicates(t *testing.T) { + err := Validate(methodsCfg([]string{"GET", "get"})) + if err == nil || !strings.Contains(err.Error(), "duplicate method") { + t.Fatalf("want duplicate-method error, got %v", err) + } +} + +func TestValidate_MethodsRejectsExplicitEmptyList(t *testing.T) { + err := Validate(methodsCfg([]string{})) + if err == nil || !strings.Contains(err.Error(), "empty list") { + t.Fatalf("want empty-list error, got %v", err) + } +} + +func TestValidate_MethodsNilAllowed(t *testing.T) { + if err := Validate(methodsCfg(nil)); err != nil { + t.Fatalf("nil methods must validate: %v", err) + } +} + +func TestAllowsMethod(t *testing.T) { + cases := []struct { + name string + methods []string + method string + want bool + }{ + {"nil list allows all", nil, "DELETE", true}, + {"listed method allowed", []string{"GET", "HEAD"}, "GET", true}, + {"unlisted method denied", []string{"GET", "HEAD"}, "POST", false}, + {"non-canonical case fails closed", []string{"GET"}, "get", false}, + } + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + s := Service{Methods: tc.methods} + if got := s.AllowsMethod(tc.method); got != tc.want { + t.Fatalf("AllowsMethod(%q) with %v = %v, want %v", tc.method, tc.methods, got, tc.want) + } + }) + } +} diff --git a/internal/brokercore/brokercore.go b/internal/brokercore/brokercore.go index 7a2db044..b280406c 100644 --- a/internal/brokercore/brokercore.go +++ b/internal/brokercore/brokercore.go @@ -201,6 +201,14 @@ func WriteInjectError(w http.ResponseWriter, err error, targetHost, vaultName, b case errors.Is(err, ErrServiceDisabled): writeProxyErrorWithHelp(w, http.StatusForbidden, "service_disabled", fmt.Sprintf("Broker service matching host %q in vault %q is currently disabled", targetHost, vaultName), baseURL) + case errors.Is(err, ErrMethodNotAllowed): + msg := fmt.Sprintf("Request method is not allowed by the broker service matching host %q in vault %q", targetHost, vaultName) + var mna *MethodNotAllowedError + if errors.As(err, &mna) { + msg = fmt.Sprintf("Method %s is not allowed by the broker service matching host %q in vault %q (allowed: %s)", + mna.Method, targetHost, vaultName, strings.Join(mna.Allowed, ", ")) + } + writeProxyErrorWithHelp(w, http.StatusForbidden, "method_not_allowed", msg, baseURL) case errors.Is(err, ErrOAuthNotConnected): writeProxyErrorWithHelp(w, http.StatusBadGateway, "oauth_not_connected", "OAuth credential is approved but not yet connected — complete the connection in the Agent Vault dashboard", baseURL) diff --git a/internal/brokercore/credential.go b/internal/brokercore/credential.go index 8b6383ee..6f99f91f 100644 --- a/internal/brokercore/credential.go +++ b/internal/brokercore/credential.go @@ -8,6 +8,7 @@ import ( "fmt" "log/slog" "net" + "strings" "time" "github.com/Infisical/agent-vault/internal/broker" @@ -57,15 +58,36 @@ type InjectResult struct { // Passthrough is set when no service matched but the unmatched-host // policy permitted forwarding. Passthrough bool + + // MethodRestricted is set when the matched service declares a Methods + // list. The MITM ingress strips method-override headers and the + // _method query parameter on such requests so an allowed method can't + // stand in for a denied one. Safe to log. + MethodRestricted bool } // CredentialProvider resolves a service for (targetHost, targetPath) in // vaultID and returns the headers to attach. targetPath must be the URL -// path only — no query, no fragment. +// path only — no query, no fragment. method is the request's HTTP method, +// checked against the matched service's Methods list. type CredentialProvider interface { - Inject(ctx context.Context, vaultID, targetHost string, targetPort int, targetPath string) (*InjectResult, error) + Inject(ctx context.Context, vaultID, targetHost string, targetPort int, targetPath, method string) (*InjectResult, error) +} + +// MethodNotAllowedError reports the denied method and the methods the +// matched service permits. Unwraps to ErrMethodNotAllowed. +type MethodNotAllowedError struct { + Method string + Allowed []string } +func (e *MethodNotAllowedError) Error() string { + return fmt.Sprintf("brokercore: method %s not allowed by broker service (allowed: %s)", + e.Method, strings.Join(e.Allowed, ", ")) +} + +func (e *MethodNotAllowedError) Unwrap() error { return ErrMethodNotAllowed } + // CredentialStore is the minimal store surface used by StoreCredentialProvider. type CredentialStore interface { GetBrokerConfig(ctx context.Context, vaultID string) (*store.BrokerConfig, error) @@ -108,8 +130,10 @@ func NewStoreCredentialProvider(s CredentialStore, encKey []byte) *StoreCredenti // Inject matches (targetHost, targetPath) and resolves the matched // service's auth into HTTP headers. targetHost may include a port — // stripped before matching. Pass "/" for targetPath when no path is -// meaningful. -func (p *StoreCredentialProvider) Inject(ctx context.Context, vaultID, targetHost string, targetPort int, targetPath string) (*InjectResult, error) { +// meaningful. method is checked against the matched service's Methods +// list after selection, parallel to the IsEnabled check — a +// method-excluded winner hard-fails with no fall-through. +func (p *StoreCredentialProvider) Inject(ctx context.Context, vaultID, targetHost string, targetPort int, targetPath, method string) (*InjectResult, error) { // A missing row is equivalent to an empty services list — fall // through to the unmatched-host policy. Any other error fails closed // so a transient store failure can't silently strip enforcement. @@ -155,6 +179,12 @@ func (p *StoreCredentialProvider) Inject(ctx context.Context, vaultID, targetHos if !matched.IsEnabled() { return nil, ErrServiceDisabled } + // The method check runs here, before either credential path (auth + // headers or substitutions) resolves, so a denied request receives + // the credential in no form. + if !matched.AllowsMethod(method) { + return nil, &MethodNotAllowedError{Method: method, Allowed: matched.Methods} + } slog.Default().Debug("broker matched", slog.String("vault", vaultID), slog.String("service", matched.Name), @@ -210,11 +240,12 @@ func (p *StoreCredentialProvider) Inject(ctx context.Context, vaultID, targetHos // Capture non-secret metadata up front so a downstream credential-missing // error still carries it for diagnostic logging. result := &InjectResult{ - MatchedName: matched.Name, - MatchedHost: matched.Host, - MatchedPath: matched.Path, - MatchedPort: matched.Port, - CredentialKeys: matched.CredentialKeys(), + MatchedName: matched.Name, + MatchedHost: matched.Host, + MatchedPath: matched.Path, + MatchedPort: matched.Port, + CredentialKeys: matched.CredentialKeys(), + MethodRestricted: len(matched.Methods) > 0, } // Resolve substitutions before auth so passthrough services (which diff --git a/internal/brokercore/credential_methods_test.go b/internal/brokercore/credential_methods_test.go new file mode 100644 index 00000000..a44f22d1 --- /dev/null +++ b/internal/brokercore/credential_methods_test.go @@ -0,0 +1,103 @@ +package brokercore + +import ( + "context" + "errors" + "testing" + + "github.com/Infisical/agent-vault/internal/broker" +) + +func TestInject_MethodAllowed(t *testing.T) { + key32 := make32(0x51) + f := newFakeCredStore() + f.setServices(t, "v1", []broker.Service{{ + Host: "api.example.com", + Methods: []string{"GET", "HEAD"}, + Auth: broker.Auth{Type: "bearer", Token: "MY_TOKEN"}, + }}) + f.setCred(t, key32, "v1", "MY_TOKEN", "s3cret") + + p := NewStoreCredentialProvider(f, key32) + res, err := p.Inject(context.Background(), "v1", "api.example.com", 0, "/", "GET") + if err != nil { + t.Fatalf("unexpected err: %v", err) + } + if res.Headers["Authorization"] != "Bearer s3cret" { + t.Fatalf("got Authorization=%q", res.Headers["Authorization"]) + } + if !res.MethodRestricted { + t.Fatal("MethodRestricted should be true for a service with a Methods list") + } +} + +func TestInject_MethodDenied(t *testing.T) { + key32 := make32(0x52) + f := newFakeCredStore() + f.setServices(t, "v1", []broker.Service{{ + Host: "api.example.com", + Methods: []string{"GET", "HEAD"}, + Auth: broker.Auth{Type: "bearer", Token: "MY_TOKEN"}, + }}) + f.setCred(t, key32, "v1", "MY_TOKEN", "s3cret") + + p := NewStoreCredentialProvider(f, key32) + _, err := p.Inject(context.Background(), "v1", "api.example.com", 0, "/", "DELETE") + if !errors.Is(err, ErrMethodNotAllowed) { + t.Fatalf("want ErrMethodNotAllowed, got %v", err) + } + var mna *MethodNotAllowedError + if !errors.As(err, &mna) { + t.Fatalf("want *MethodNotAllowedError, got %T", err) + } + if mna.Method != "DELETE" || len(mna.Allowed) != 2 || mna.Allowed[0] != "GET" { + t.Fatalf("unexpected error detail: %+v", mna) + } + // The check must run before credential resolution: a denied request + // must not decrypt or touch the credential in any form. + if f.getCredentialCalls != 0 { + t.Fatalf("credential store consulted %d times for a denied method", f.getCredentialCalls) + } +} + +func TestInject_MethodDeniedBlocksSubstitutionsOnPassthrough(t *testing.T) { + key32 := make32(0x53) + f := newFakeCredStore() + f.setServices(t, "v1", []broker.Service{{ + Host: "api.example.com", + Methods: []string{"GET"}, + Auth: broker.Auth{Type: "passthrough"}, + Substitutions: []broker.Substitution{{ + Key: "SIGNING_KEY", Placeholder: "__signing_key__", + }}, + }}) + f.setCred(t, key32, "v1", "SIGNING_KEY", "sig-secret") + + p := NewStoreCredentialProvider(f, key32) + _, err := p.Inject(context.Background(), "v1", "api.example.com", 0, "/", "POST") + if !errors.Is(err, ErrMethodNotAllowed) { + t.Fatalf("want ErrMethodNotAllowed, got %v", err) + } + if f.getCredentialCalls != 0 { + t.Fatalf("substitution credential resolved %d times for a denied method", f.getCredentialCalls) + } +} + +func TestInject_NoMethodsAllowsAll(t *testing.T) { + key32 := make32(0x54) + f := newFakeCredStore() + f.setServices(t, "v1", []broker.Service{{ + Host: "api.example.com", + Auth: broker.Auth{Type: "bearer", Token: "MY_TOKEN"}, + }}) + f.setCred(t, key32, "v1", "MY_TOKEN", "s3cret") + + p := NewStoreCredentialProvider(f, key32) + res, err := p.Inject(context.Background(), "v1", "api.example.com", 0, "/", "DELETE") + if err != nil { + t.Fatalf("unexpected err: %v", err) + } + if res.MethodRestricted { + t.Fatal("MethodRestricted should be false when the service has no Methods list") + } +} diff --git a/internal/brokercore/credential_test.go b/internal/brokercore/credential_test.go index 082d10f0..b64639a4 100644 --- a/internal/brokercore/credential_test.go +++ b/internal/brokercore/credential_test.go @@ -99,7 +99,7 @@ func TestInject_BearerHappyPath(t *testing.T) { f.setCred(t, key32, "v1", "MY_TOKEN", "s3cret") p := NewStoreCredentialProvider(f, key32) - res, err := p.Inject(context.Background(), "v1", "api.example.com", 0, "/") + res, err := p.Inject(context.Background(), "v1", "api.example.com", 0, "/", "GET") if err != nil { t.Fatalf("unexpected err: %v", err) } @@ -119,7 +119,7 @@ func TestInject_BasicHappyPath(t *testing.T) { f.setCred(t, key32, "v1", "PASS", "wonderland") p := NewStoreCredentialProvider(f, key32) - res, err := p.Inject(context.Background(), "v1", "api.example.com", 0, "/") + res, err := p.Inject(context.Background(), "v1", "api.example.com", 0, "/", "GET") if err != nil { t.Fatalf("unexpected err: %v", err) } @@ -139,7 +139,7 @@ func TestInject_APIKeyCustomHeader(t *testing.T) { f.setCred(t, key32, "v1", "STRIPE_KEY", "live123") p := NewStoreCredentialProvider(f, key32) - res, err := p.Inject(context.Background(), "v1", "api.example.com", 0, "/") + res, err := p.Inject(context.Background(), "v1", "api.example.com", 0, "/", "GET") if err != nil { t.Fatalf("unexpected err: %v", err) } @@ -162,7 +162,7 @@ func TestInject_CustomHeaders(t *testing.T) { f.setCred(t, key32, "v1", "TENANT", "42") p := NewStoreCredentialProvider(f, key32) - res, err := p.Inject(context.Background(), "v1", "api.example.com", 0, "/") + res, err := p.Inject(context.Background(), "v1", "api.example.com", 0, "/", "GET") if err != nil { t.Fatalf("unexpected err: %v", err) } @@ -184,7 +184,7 @@ func TestInject_StripsPortForMatching(t *testing.T) { f.setCred(t, key32, "v1", "TOK", "v") p := NewStoreCredentialProvider(f, key32) - res, err := p.Inject(context.Background(), "v1", "api.example.com:443", 443, "/") + res, err := p.Inject(context.Background(), "v1", "api.example.com:443", 443, "/", "GET") if err != nil { t.Fatalf("unexpected err: %v", err) } @@ -209,7 +209,7 @@ func TestInject_HealsLegacyUnnamedServiceMatchedName(t *testing.T) { f.setCred(t, key32, "v1", "TOK", "s3cret") p := NewStoreCredentialProvider(f, key32) - res, err := p.Inject(context.Background(), "v1", "api.example.com", 0, "/") + res, err := p.Inject(context.Background(), "v1", "api.example.com", 0, "/", "GET") if err != nil { t.Fatalf("unexpected err: %v", err) } @@ -228,7 +228,7 @@ func TestInject_WildcardMatch(t *testing.T) { f.setCred(t, key32, "v1", "GH", "ghp_abc") p := NewStoreCredentialProvider(f, key32) - res, err := p.Inject(context.Background(), "v1", "api.github.com", 0, "/") + res, err := p.Inject(context.Background(), "v1", "api.github.com", 0, "/", "GET") if err != nil { t.Fatalf("unexpected err: %v", err) } @@ -275,7 +275,7 @@ func TestInject_PathBasedDisambiguation(t *testing.T) { {"/api/apps.connections.open", "Bearer xapp-conn", "slack-conn", "/api/apps.connections.*"}, } for _, tc := range cases { - res, err := p.Inject(context.Background(), "v1", "slack.com", 0, tc.path) + res, err := p.Inject(context.Background(), "v1", "slack.com", 0, tc.path, "GET") if err != nil { t.Fatalf("path %q: unexpected err: %v", tc.path, err) } @@ -299,7 +299,7 @@ func TestInject_UnmatchedHost_DefaultPassthrough(t *testing.T) { // no matching service forwards without injection. f := newFakeCredStore() p := NewStoreCredentialProvider(f, make32(0x77)) - res, err := p.Inject(context.Background(), "v1", "api.example.com", 0, "/") + res, err := p.Inject(context.Background(), "v1", "api.example.com", 0, "/", "GET") if err != nil { t.Fatalf("unexpected err: %v", err) } @@ -326,7 +326,7 @@ func TestInject_UnmatchedHost_HostMissPassthrough(t *testing.T) { f.setCred(t, key32, "v1", "T", "x") p := NewStoreCredentialProvider(f, key32) - res, err := p.Inject(context.Background(), "v1", "other.example.com", 0, "/") + res, err := p.Inject(context.Background(), "v1", "other.example.com", 0, "/", "GET") if err != nil { t.Fatalf("unexpected err: %v", err) } @@ -342,7 +342,7 @@ func TestInject_UnmatchedHost_DenyPolicy(t *testing.T) { f := newFakeCredStore() f.policy = PolicyDeny p := NewStoreCredentialProvider(f, make32(0x77)) - _, err := p.Inject(context.Background(), "v1", "api.example.com", 0, "/") + _, err := p.Inject(context.Background(), "v1", "api.example.com", 0, "/", "GET") if !errors.Is(err, ErrServiceNotFound) { t.Fatalf("expected ErrServiceNotFound under deny policy, got %v", err) } @@ -359,7 +359,7 @@ func TestInject_UnmatchedHost_HostMissDeny(t *testing.T) { f.setCred(t, key32, "v1", "T", "x") p := NewStoreCredentialProvider(f, key32) - _, err := p.Inject(context.Background(), "v1", "other.example.com", 0, "/") + _, err := p.Inject(context.Background(), "v1", "other.example.com", 0, "/", "GET") if !errors.Is(err, ErrServiceNotFound) { t.Fatalf("expected ErrServiceNotFound under deny policy, got %v", err) } @@ -373,7 +373,7 @@ func TestInject_GetBrokerConfigError_FailsClosed(t *testing.T) { f := newFakeCredStore() f.brokerCfgErr = errors.New("transient sqlite I/O error") p := NewStoreCredentialProvider(f, make32(0xAB)) - _, err := p.Inject(context.Background(), "v1", "api.example.com", 0, "/") + _, err := p.Inject(context.Background(), "v1", "api.example.com", 0, "/", "GET") if !errors.Is(err, ErrServiceNotFound) { t.Fatalf("expected ErrServiceNotFound on store error, got %v", err) } @@ -388,7 +388,7 @@ func TestInject_CredentialMissing(t *testing.T) { }}) p := NewStoreCredentialProvider(f, key32) - _, err := p.Inject(context.Background(), "v1", "api.example.com", 0, "/") + _, err := p.Inject(context.Background(), "v1", "api.example.com", 0, "/", "GET") if !errors.Is(err, ErrCredentialMissing) { t.Fatalf("expected ErrCredentialMissing, got %v", err) } @@ -406,7 +406,7 @@ func TestInject_DecryptFails(t *testing.T) { f.setCred(t, encKey, "v1", "TOK", "secret") p := NewStoreCredentialProvider(f, wrongKey) - _, err := p.Inject(context.Background(), "v1", "api.example.com", 0, "/") + _, err := p.Inject(context.Background(), "v1", "api.example.com", 0, "/", "GET") if !errors.Is(err, ErrCredentialMissing) { t.Fatalf("expected ErrCredentialMissing, got %v", err) } @@ -420,7 +420,7 @@ func TestInject_Passthrough(t *testing.T) { }}) p := NewStoreCredentialProvider(f, make32(0xCC)) - res, err := p.Inject(context.Background(), "v1", "api.example.com", 0, "/") + res, err := p.Inject(context.Background(), "v1", "api.example.com", 0, "/", "GET") if err != nil { t.Fatalf("unexpected err: %v", err) } @@ -450,7 +450,7 @@ func TestInject_ServiceDisabled(t *testing.T) { f.setCred(t, key32, "v1", "TOK", "x") p := NewStoreCredentialProvider(f, key32) - _, err := p.Inject(context.Background(), "v1", "api.example.com", 0, "/") + _, err := p.Inject(context.Background(), "v1", "api.example.com", 0, "/", "GET") if !errors.Is(err, ErrServiceDisabled) { t.Fatalf("expected ErrServiceDisabled, got %v", err) } @@ -468,7 +468,7 @@ func TestInject_ServiceDisabled_Passthrough(t *testing.T) { Auth: broker.Auth{Type: "passthrough"}, }}) p := NewStoreCredentialProvider(f, make32(0xEF)) - _, err := p.Inject(context.Background(), "v1", "api.example.com", 0, "/") + _, err := p.Inject(context.Background(), "v1", "api.example.com", 0, "/", "GET") if !errors.Is(err, ErrServiceDisabled) { t.Fatalf("expected ErrServiceDisabled for disabled passthrough, got %v", err) } @@ -486,7 +486,7 @@ func TestInject_EnabledExplicitTrue(t *testing.T) { f.setCred(t, key32, "v1", "TOK", "v") p := NewStoreCredentialProvider(f, key32) - res, err := p.Inject(context.Background(), "v1", "api.example.com", 0, "/") + res, err := p.Inject(context.Background(), "v1", "api.example.com", 0, "/", "GET") if err != nil { t.Fatalf("unexpected err: %v", err) } @@ -502,7 +502,7 @@ func TestInject_PassthroughPortStripped(t *testing.T) { Auth: broker.Auth{Type: "passthrough"}, }}) p := NewStoreCredentialProvider(f, make32(0xDD)) - res, err := p.Inject(context.Background(), "v1", "api.example.com:443", 443, "/") + res, err := p.Inject(context.Background(), "v1", "api.example.com:443", 443, "/", "GET") if err != nil { t.Fatalf("unexpected err: %v", err) } @@ -530,7 +530,7 @@ func TestInject_ResolvesSubstitutionAlongsideAuth(t *testing.T) { f.setCred(t, key32, "v1", "TWILIO_AUTH_TOKEN", "tok-shh") p := NewStoreCredentialProvider(f, key32) - res, err := p.Inject(context.Background(), "v1", "api.twilio.com", 0, "/") + res, err := p.Inject(context.Background(), "v1", "api.twilio.com", 0, "/", "GET") if err != nil { t.Fatalf("unexpected err: %v", err) } @@ -565,7 +565,7 @@ func TestInject_ResolvesSubstitutionOnPassthrough(t *testing.T) { f.setCred(t, key32, "v1", "TWILIO_ACCOUNT_SID", "AC12345") p := NewStoreCredentialProvider(f, key32) - res, err := p.Inject(context.Background(), "v1", "api.twilio.com", 0, "/") + res, err := p.Inject(context.Background(), "v1", "api.twilio.com", 0, "/", "GET") if err != nil { t.Fatalf("unexpected err: %v", err) } @@ -589,7 +589,7 @@ func TestInject_SubstitutionMissingCredentialErrorsLikeAuth(t *testing.T) { }}) // No credential set → lookup returns "not found". p := NewStoreCredentialProvider(f, key32) - _, err := p.Inject(context.Background(), "v1", "api.twilio.com", 0, "/") + _, err := p.Inject(context.Background(), "v1", "api.twilio.com", 0, "/", "GET") if !errors.Is(err, ErrCredentialMissing) { t.Fatalf("expected ErrCredentialMissing, got %v", err) } @@ -613,7 +613,7 @@ func TestInject_AuthFailureLeavesSubstitutionsNil(t *testing.T) { // MISSING_AUTH_KEY is intentionally not set. p := NewStoreCredentialProvider(f, key32) - res, err := p.Inject(context.Background(), "v1", "api.twilio.com", 0, "/") + res, err := p.Inject(context.Background(), "v1", "api.twilio.com", 0, "/", "GET") if !errors.Is(err, ErrCredentialMissing) { t.Fatalf("expected ErrCredentialMissing, got %v", err) } @@ -637,7 +637,7 @@ func TestInject_PortMatch(t *testing.T) { f.setCred(t, key32, "v1", "TOK", "secret") p := NewStoreCredentialProvider(f, key32) - res, err := p.Inject(context.Background(), "v1", "api.example.com", 8080, "/") + res, err := p.Inject(context.Background(), "v1", "api.example.com", 8080, "/", "GET") if err != nil { t.Fatalf("unexpected err: %v", err) } @@ -661,7 +661,7 @@ func TestInject_PortMismatch(t *testing.T) { f.setCred(t, key32, "v1", "TOK", "secret") p := NewStoreCredentialProvider(f, key32) - res, err := p.Inject(context.Background(), "v1", "api.example.com", 9090, "/") + res, err := p.Inject(context.Background(), "v1", "api.example.com", 9090, "/", "GET") if err != nil { t.Fatalf("unexpected err: %v", err) } @@ -682,7 +682,7 @@ func TestInject_PortSpecificWinsOverGeneral(t *testing.T) { f.setCred(t, key32, "v1", "SPECIFIC_TOK", "specific-secret") p := NewStoreCredentialProvider(f, key32) - res, err := p.Inject(context.Background(), "v1", "api.example.com", 443, "/") + res, err := p.Inject(context.Background(), "v1", "api.example.com", 443, "/", "GET") if err != nil { t.Fatalf("unexpected err: %v", err) } @@ -704,7 +704,7 @@ func TestInject_NilPortMatchesAnyPort(t *testing.T) { f.setCred(t, key32, "v1", "TOK", "secret") p := NewStoreCredentialProvider(f, key32) - res, err := p.Inject(context.Background(), "v1", "api.example.com", 8080, "/") + res, err := p.Inject(context.Background(), "v1", "api.example.com", 8080, "/", "GET") if err != nil { t.Fatalf("unexpected err: %v", err) } @@ -727,7 +727,7 @@ func TestInject_CredentialKeysIncludesSubstitution(t *testing.T) { // No SID credential → expect ErrCredentialMissing, but CredentialKeys // must already be populated for diagnostic logging. p := NewStoreCredentialProvider(f, key32) - res, err := p.Inject(context.Background(), "v1", "api.twilio.com", 0, "/") + res, err := p.Inject(context.Background(), "v1", "api.twilio.com", 0, "/", "GET") if !errors.Is(err, ErrCredentialMissing) { t.Fatalf("expected ErrCredentialMissing, got %v", err) } @@ -762,7 +762,7 @@ func TestInject_DynamicFallback_Resolves(t *testing.T) { p := NewStoreCredentialProvider(f, key32) p.Dynamic = dyn - res, err := p.Inject(context.Background(), "v1", "db.example.com", 0, "/") + res, err := p.Inject(context.Background(), "v1", "db.example.com", 0, "/", "GET") if err != nil { t.Fatalf("unexpected err: %v", err) } @@ -786,7 +786,7 @@ func TestInject_DynamicFallback_NotDynamic_StillMissing(t *testing.T) { p := NewStoreCredentialProvider(f, key32) p.Dynamic = dyn - _, err := p.Inject(context.Background(), "v1", "db.example.com", 0, "/") + _, err := p.Inject(context.Background(), "v1", "db.example.com", 0, "/", "GET") if !errors.Is(err, ErrCredentialMissing) { t.Fatalf("expected ErrCredentialMissing, got %v", err) } @@ -804,7 +804,7 @@ func TestInject_DynamicFallback_ErrorPropagates(t *testing.T) { p := NewStoreCredentialProvider(f, key32) p.Dynamic = dyn - _, err := p.Inject(context.Background(), "v1", "db.example.com", 0, "/") + _, err := p.Inject(context.Background(), "v1", "db.example.com", 0, "/", "GET") if err == nil { t.Fatalf("expected error to propagate") } diff --git a/internal/brokercore/errors.go b/internal/brokercore/errors.go index 26cd6936..12615b1f 100644 --- a/internal/brokercore/errors.go +++ b/internal/brokercore/errors.go @@ -43,6 +43,13 @@ var ( // configured". Callers surface 403 with error code "service_disabled". ErrServiceDisabled = errors.New("brokercore: broker service is disabled") + // ErrMethodNotAllowed means the matched broker service restricts HTTP + // methods and the request's method is not in its list. Like + // ErrServiceDisabled this hard-fails the matched service — there is no + // fall-through to a broader sibling. Callers surface 403 with error + // code "method_not_allowed". + ErrMethodNotAllowed = errors.New("brokercore: request method not allowed by broker service") + // ErrOAuthNotConnected means the credential is an OAuth type but // the consent flow hasn't completed yet (no access token stored). ErrOAuthNotConnected = errors.New("brokercore: oauth credential not yet connected") diff --git a/internal/brokercore/logging_test.go b/internal/brokercore/logging_test.go index a301ef67..473823ac 100644 --- a/internal/brokercore/logging_test.go +++ b/internal/brokercore/logging_test.go @@ -78,7 +78,7 @@ func TestLogProxyEvent_NoSecretLeak(t *testing.T) { } provider := NewStoreCredentialProvider(f, encKey) - result, err := provider.Inject(context.Background(), "v1", "api.example.com", 0, "/") + result, err := provider.Inject(context.Background(), "v1", "api.example.com", 0, "/", "GET") if err != nil { t.Fatalf("Inject: %v", err) } @@ -129,7 +129,7 @@ func TestLogProxyEvent_CredentialMissingCarriesMetadata(t *testing.T) { // Deliberately don't seed MISSING_TOKEN — Resolve will fail. provider := NewStoreCredentialProvider(f, encKey) - result, err := provider.Inject(context.Background(), "v1", "api.example.com", 0, "/") + result, err := provider.Inject(context.Background(), "v1", "api.example.com", 0, "/", "GET") if err == nil { t.Fatal("expected ErrCredentialMissing, got nil") } diff --git a/internal/mitm/forward.go b/internal/mitm/forward.go index c2525110..04694b6a 100644 --- a/internal/mitm/forward.go +++ b/internal/mitm/forward.go @@ -238,7 +238,7 @@ func (p *Proxy) forwardRequest( return } - inject, err := p.creds.Inject(r.Context(), scope.VaultID, host, port, r.URL.Path) + inject, err := p.creds.Inject(r.Context(), scope.VaultID, host, port, r.URL.Path, r.Method) if inject != nil { event.MatchedService = inject.MatchedName event.MatchedHost = inject.MatchedHost @@ -250,16 +250,27 @@ func (p *Proxy) forwardRequest( if err != nil { errCode := "no_match" status := http.StatusForbidden - if errors.Is(err, brokercore.ErrCredentialMissing) { + switch { + case errors.Is(err, brokercore.ErrCredentialMissing): errCode = "credential_not_found" status = http.StatusBadGateway brokercore.LogCredentialMissing(p.logger, scope.VaultID, event.MatchedService, event.CredentialKeys) + case errors.Is(err, brokercore.ErrMethodNotAllowed): + errCode = "method_not_allowed" } brokercore.WriteInjectError(w, err, target, scope.VaultName, p.baseURL) emit(status, errCode) return } + // A method-restricted service also has the well-known method-override + // spoofing surfaces stripped, so an allowed method can't stand in for + // a denied one. The _method form carried in a POST body is not + // inspected. + if inject.MethodRestricted { + outURL.RawQuery = stripMethodOverrideParam(outURL.RawQuery) + } + var body io.ReadCloser var contentLength int64 @@ -291,11 +302,16 @@ func (p *Proxy) forwardRequest( wsUpgrade := isWebSocketUpgrade(r) + var extraStrip []string + if inject.MethodRestricted { + extraStrip = methodOverrideHeaders + } if wsUpgrade { copyWebSocketHandshakeHeaders(r.Header, outReq.Header) - brokercore.ApplyInjection(r.Header, outReq.Header, inject, websocketHandshakeHeaderNames...) + strip := append(append([]string(nil), websocketHandshakeHeaderNames...), extraStrip...) + brokercore.ApplyInjection(r.Header, outReq.Header, inject, strip...) } else { - brokercore.ApplyInjection(r.Header, outReq.Header, inject) + brokercore.ApplyInjection(r.Header, outReq.Header, inject, extraStrip...) } if err := brokercore.ApplySubstitutions(outReq.URL, outReq.Header, inject.Substitutions); err != nil { @@ -353,7 +369,7 @@ func (p *Proxy) forwardRequest( if resp.StatusCode == http.StatusUnauthorized && inject != nil && !inject.Passthrough && (r.Method == http.MethodGet || r.Method == http.MethodHead) { _ = resp.Body.Close() - retryInject, retryErr := p.creds.Inject(r.Context(), scope.VaultID, host, port, r.URL.Path) + retryInject, retryErr := p.creds.Inject(r.Context(), scope.VaultID, host, port, r.URL.Path, r.Method) if retryErr == nil && retryInject != nil && retryInject.Headers != nil { retryReq := outReq.Clone(outReq.Context()) for k, v := range retryInject.Headers { @@ -426,6 +442,33 @@ func (p *Proxy) forwardRequest( emit(resp.StatusCode, "") } +// methodOverrideHeaders are the well-known headers some upstreams honor +// to override the effective HTTP method. Stripped on method-restricted +// services so an allowed method can't tunnel a denied one. +var methodOverrideHeaders = []string{"X-Http-Method-Override", "X-Method-Override", "X-Http-Method"} + +// stripMethodOverrideParam removes any _method key from a raw query +// string. The surviving pairs are preserved byte-for-byte — no +// re-encoding or reordering — so signed URLs keep their signatures. +func stripMethodOverrideParam(rawQuery string) string { + if !strings.Contains(rawQuery, "_method") { + return rawQuery + } + parts := strings.Split(rawQuery, "&") + kept := parts[:0] + for _, p := range parts { + key := p + if i := strings.IndexByte(p, '='); i >= 0 { + key = p[:i] + } + if key == "_method" { + continue + } + kept = append(kept, p) + } + return strings.Join(kept, "&") +} + // knownAPIKeyHeaders are non-Authorization headers that commonly carry // API keys. Checked by exact canonical match -- no heuristic scanning. var knownAPIKeyHeaders = []string{"X-Api-Key", "Api-Key"} diff --git a/internal/mitm/forward_methods_test.go b/internal/mitm/forward_methods_test.go new file mode 100644 index 00000000..969d1231 --- /dev/null +++ b/internal/mitm/forward_methods_test.go @@ -0,0 +1,115 @@ +package mitm + +import ( + "crypto/tls" + "crypto/x509" + "net" + "net/http" + "net/http/httptest" + "net/url" + "strings" + "testing" + + "github.com/Infisical/agent-vault/internal/brokercore" +) + +func TestStripMethodOverrideParam(t *testing.T) { + cases := []struct { + name string + in string + want string + }{ + {"empty", "", ""}, + {"no override", "a=1&b=2", "a=1&b=2"}, + {"only override", "_method=DELETE", ""}, + {"override first", "_method=DELETE&a=1", "a=1"}, + {"override middle", "a=1&_method=DELETE&b=2", "a=1&b=2"}, + {"override last", "a=1&_method=DELETE", "a=1"}, + {"override without value", "a=1&_method", "a=1"}, + {"similar keys untouched", "x_method=1&_methodx=2&a=_method", "x_method=1&_methodx=2&a=_method"}, + {"encoding preserved", "sig=a%2Fb%3D&_method=PUT", "sig=a%2Fb%3D"}, + } + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + if got := stripMethodOverrideParam(tc.in); got != tc.want { + t.Fatalf("stripMethodOverrideParam(%q) = %q, want %q", tc.in, got, tc.want) + } + }) + } +} + +// TestMITMStripsMethodOverrideOnRestrictedService drives a request with a +// method-override header and a _method query param through the proxy +// twice: against a method-restricted service (both must be stripped +// before the upstream sees them) and against an unrestricted one (both +// must pass through untouched). +func TestMITMStripsMethodOverrideOnRestrictedService(t *testing.T) { + for _, restricted := range []bool{true, false} { + name := "restricted" + if !restricted { + name = "unrestricted" + } + t.Run(name, func(t *testing.T) { + var sawOverride, sawQuery string + upstream := httptest.NewTLSServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + sawOverride = r.Header.Get("X-Http-Method-Override") + sawQuery = r.URL.RawQuery + })) + defer upstream.Close() + + upstreamAuthority := strings.TrimPrefix(upstream.URL, "https://") + upstreamHost, _, _ := net.SplitHostPort(upstreamAuthority) + + sr := validTokenResolver("av_sess_ok", + &brokercore.ProxyScope{VaultID: "v1", VaultName: "default", VaultRole: "proxy"}) + cp := &fakeCredProvider{byHost: map[string]fakeInjectResult{ + upstreamHost: {result: &brokercore.InjectResult{ + Headers: map[string]string{"Authorization": "Bearer injected"}, + MethodRestricted: restricted, + }}, + }} + + proxyURL, clientRoots, p := setupProxy(t, sr, cp) + + upstreamRoots := x509.NewCertPool() + upstreamRoots.AddCert(upstream.Certificate()) + p.upstream.TLSClientConfig = &tls.Config{ + MinVersion: tls.VersionTLS12, + RootCAs: upstreamRoots, + } + + client := newTrustingClient(proxyURL, url.User("av_sess_ok"), clientRoots) + + req, err := http.NewRequest("GET", upstream.URL+"/ping?_method=DELETE&keep=1", nil) + if err != nil { + t.Fatalf("new request: %v", err) + } + req.Header.Set("X-HTTP-Method-Override", "DELETE") + + resp, err := client.Do(req) + if err != nil { + t.Fatalf("client.Do: %v", err) + } + defer resp.Body.Close() + if resp.StatusCode != http.StatusOK { + t.Fatalf("status = %d, want 200", resp.StatusCode) + } + + if restricted { + if sawOverride != "" { + t.Fatalf("upstream saw X-Http-Method-Override %q, want stripped", sawOverride) + } + if sawQuery != "keep=1" { + t.Fatalf("upstream saw query %q, want _method stripped and keep=1 preserved", sawQuery) + } + } else { + if sawOverride != "DELETE" { + t.Fatalf("upstream saw X-Http-Method-Override %q, want passthrough", sawOverride) + } + if sawQuery != "_method=DELETE&keep=1" { + t.Fatalf("upstream saw query %q, want untouched", sawQuery) + } + } + }) + } +} diff --git a/internal/mitm/proxy_test.go b/internal/mitm/proxy_test.go index 7d6c4e04..0e94524b 100644 --- a/internal/mitm/proxy_test.go +++ b/internal/mitm/proxy_test.go @@ -68,7 +68,7 @@ type fakeInjectResult struct { err error } -func (f *fakeCredProvider) Inject(_ context.Context, _, targetHost string, targetPort int, _ string) (*brokercore.InjectResult, error) { +func (f *fakeCredProvider) Inject(_ context.Context, _, targetHost string, targetPort int, _, _ string) (*brokercore.InjectResult, error) { host := targetHost if h, _, err := net.SplitHostPort(targetHost); err == nil { host = h