From d89ac24af2fe61623bf95a41e1edc0a5a1e5ad05 Mon Sep 17 00:00:00 2001 From: Josh Siviour Date: Sun, 2 Aug 2026 08:04:12 +1000 Subject: [PATCH] fix(gateway): enforce iam:PassRole on ECS task and service role ARNs (#632) RegisterTaskDefinition let any caller with ecs:RegisterTaskDefinition name an arbitrary taskRoleArn/executionRoleArn, and RunTask/StartTask/CreateService/ UpdateService could launch tasks under a previously registered definition's roles, all without holding iam:PassRole on those roles. Mirror the EC2 gateway's PassRoleChecker closure pattern to close both paths. --- spinifex/gateway/ecs.go | 9 +- spinifex/gateway/ecs/actions.go | 154 +++++++++++---- spinifex/gateway/ecs/actions_test.go | 274 ++++++++++++++++++++++++++ spinifex/gateway/ecs/handler.go | 14 +- spinifex/gateway/ecs/handler_test.go | 4 +- spinifex/gateway/ecs_passrole_test.go | 137 +++++++++++++ 6 files changed, 546 insertions(+), 46 deletions(-) create mode 100644 spinifex/gateway/ecs/actions_test.go create mode 100644 spinifex/gateway/ecs_passrole_test.go diff --git a/spinifex/gateway/ecs.go b/spinifex/gateway/ecs.go index 77a8555d2..e3aa7c23d 100644 --- a/spinifex/gateway/ecs.go +++ b/spinifex/gateway/ecs.go @@ -52,7 +52,14 @@ func (gw *GatewayConfig) ECS_Request(w http.ResponseWriter, r *http.Request) err return errors.New(awserrors.ErrorInvalidParameterValue) } - output, err := handler(r.Context(), gw.NATSConn, accountID, body) + // Mirrors gateway/ec2.go's RunInstances/AssociateIamInstanceProfile closures: + // enforce iam:PassRole against the caller's identity on this request, for + // whichever role ARN the action later resolves. + passRoleCheck := func(roleARN string) error { + return gw.checkPolicyResource(r, "iam", "PassRole", roleARN) + } + + output, err := handler(r.Context(), gw.NATSConn, accountID, body, passRoleCheck) if err != nil { return err } diff --git a/spinifex/gateway/ecs/actions.go b/spinifex/gateway/ecs/actions.go index 69085098d..4d3f3aed3 100644 --- a/spinifex/gateway/ecs/actions.go +++ b/spinifex/gateway/ecs/actions.go @@ -4,6 +4,7 @@ import ( "context" "encoding/json" + "github.com/aws/aws-sdk-go/aws" "github.com/aws/aws-sdk-go/service/ecs" handlers_ecs "github.com/mulgadc/spinifex/spinifex/handlers/ecs" "github.com/nats-io/nats.go" @@ -17,9 +18,43 @@ func unmarshalIfBody(body []byte, out any) error { return json.Unmarshal(body, out) } +// checkTaskDefinitionRoles enforces iam:PassRole on every role ARN a task +// definition names. A nil/empty ARN is skipped — there is nothing to pass, +// matching how the EC2 path treats a roleless instance profile. +func checkTaskDefinitionRoles(passRoleCheck PassRoleChecker, roleARNs ...*string) error { + if passRoleCheck == nil { + return nil + } + for _, arn := range roleARNs { + if roleARN := aws.StringValue(arn); roleARN != "" { + if err := passRoleCheck(roleARN); err != nil { + return err + } + } + } + return nil +} + +// checkTaskLaunchRoles resolves taskDefinition's role ARNs and enforces +// iam:PassRole before a launch can start tasks under them, since RunTask, +// StartTask, CreateService and UpdateService only reference a task definition. +func checkTaskLaunchRoles(ctx context.Context, svc handlers_ecs.ECSService, accountID string, taskDefinition *string, passRoleCheck PassRoleChecker) error { + if passRoleCheck == nil || aws.StringValue(taskDefinition) == "" { + return nil + } + out, err := svc.DescribeTaskDefinition(ctx, &ecs.DescribeTaskDefinitionInput{TaskDefinition: taskDefinition}, accountID) + if err != nil { + return err + } + if out == nil || out.TaskDefinition == nil { + return nil + } + return checkTaskDefinitionRoles(passRoleCheck, out.TaskDefinition.TaskRoleArn, out.TaskDefinition.ExecutionRoleArn) +} + // --- Cluster --- -func CreateCluster(ctx context.Context, nc *nats.Conn, accountID string, body []byte) (any, error) { +func CreateCluster(ctx context.Context, nc *nats.Conn, accountID string, body []byte, passRoleCheck PassRoleChecker) (any, error) { input := new(ecs.CreateClusterInput) if err := unmarshalIfBody(body, input); err != nil { return nil, err @@ -27,7 +62,7 @@ func CreateCluster(ctx context.Context, nc *nats.Conn, accountID string, body [] return handlers_ecs.NewNATSECSService(nc).CreateCluster(ctx, input, accountID) } -func DeleteCluster(ctx context.Context, nc *nats.Conn, accountID string, body []byte) (any, error) { +func DeleteCluster(ctx context.Context, nc *nats.Conn, accountID string, body []byte, passRoleCheck PassRoleChecker) (any, error) { input := new(ecs.DeleteClusterInput) if err := unmarshalIfBody(body, input); err != nil { return nil, err @@ -35,7 +70,7 @@ func DeleteCluster(ctx context.Context, nc *nats.Conn, accountID string, body [] return handlers_ecs.NewNATSECSService(nc).DeleteCluster(ctx, input, accountID) } -func DescribeClusters(ctx context.Context, nc *nats.Conn, accountID string, body []byte) (any, error) { +func DescribeClusters(ctx context.Context, nc *nats.Conn, accountID string, body []byte, passRoleCheck PassRoleChecker) (any, error) { input := new(ecs.DescribeClustersInput) if err := unmarshalIfBody(body, input); err != nil { return nil, err @@ -43,7 +78,7 @@ func DescribeClusters(ctx context.Context, nc *nats.Conn, accountID string, body return handlers_ecs.NewNATSECSService(nc).DescribeClusters(ctx, input, accountID) } -func ListClusters(ctx context.Context, nc *nats.Conn, accountID string, body []byte) (any, error) { +func ListClusters(ctx context.Context, nc *nats.Conn, accountID string, body []byte, passRoleCheck PassRoleChecker) (any, error) { input := new(ecs.ListClustersInput) if err := unmarshalIfBody(body, input); err != nil { return nil, err @@ -53,15 +88,18 @@ func ListClusters(ctx context.Context, nc *nats.Conn, accountID string, body []b // --- Task definition --- -func RegisterTaskDefinition(ctx context.Context, nc *nats.Conn, accountID string, body []byte) (any, error) { +func RegisterTaskDefinition(ctx context.Context, nc *nats.Conn, accountID string, body []byte, passRoleCheck PassRoleChecker) (any, error) { input := new(ecs.RegisterTaskDefinitionInput) if err := unmarshalIfBody(body, input); err != nil { return nil, err } + if err := checkTaskDefinitionRoles(passRoleCheck, input.TaskRoleArn, input.ExecutionRoleArn); err != nil { + return nil, err + } return handlers_ecs.NewNATSECSService(nc).RegisterTaskDefinition(ctx, input, accountID) } -func DeregisterTaskDefinition(ctx context.Context, nc *nats.Conn, accountID string, body []byte) (any, error) { +func DeregisterTaskDefinition(ctx context.Context, nc *nats.Conn, accountID string, body []byte, passRoleCheck PassRoleChecker) (any, error) { input := new(ecs.DeregisterTaskDefinitionInput) if err := unmarshalIfBody(body, input); err != nil { return nil, err @@ -69,7 +107,7 @@ func DeregisterTaskDefinition(ctx context.Context, nc *nats.Conn, accountID stri return handlers_ecs.NewNATSECSService(nc).DeregisterTaskDefinition(ctx, input, accountID) } -func DescribeTaskDefinition(ctx context.Context, nc *nats.Conn, accountID string, body []byte) (any, error) { +func DescribeTaskDefinition(ctx context.Context, nc *nats.Conn, accountID string, body []byte, passRoleCheck PassRoleChecker) (any, error) { input := new(ecs.DescribeTaskDefinitionInput) if err := unmarshalIfBody(body, input); err != nil { return nil, err @@ -77,7 +115,7 @@ func DescribeTaskDefinition(ctx context.Context, nc *nats.Conn, accountID string return handlers_ecs.NewNATSECSService(nc).DescribeTaskDefinition(ctx, input, accountID) } -func ListTaskDefinitions(ctx context.Context, nc *nats.Conn, accountID string, body []byte) (any, error) { +func ListTaskDefinitions(ctx context.Context, nc *nats.Conn, accountID string, body []byte, passRoleCheck PassRoleChecker) (any, error) { input := new(ecs.ListTaskDefinitionsInput) if err := unmarshalIfBody(body, input); err != nil { return nil, err @@ -87,7 +125,7 @@ func ListTaskDefinitions(ctx context.Context, nc *nats.Conn, accountID string, b // --- Container instance --- -func RegisterContainerInstance(ctx context.Context, nc *nats.Conn, accountID string, body []byte) (any, error) { +func RegisterContainerInstance(ctx context.Context, nc *nats.Conn, accountID string, body []byte, passRoleCheck PassRoleChecker) (any, error) { input := new(ecs.RegisterContainerInstanceInput) if err := unmarshalIfBody(body, input); err != nil { return nil, err @@ -97,7 +135,7 @@ func RegisterContainerInstance(ctx context.Context, nc *nats.Conn, accountID str // ProvisionCapacity launches container-instance EC2 capacity into a cluster. // Input/output are the custom handlers_ecs types, not aws-sdk-go ecs shapes. -func ProvisionCapacity(ctx context.Context, nc *nats.Conn, accountID string, body []byte) (any, error) { +func ProvisionCapacity(ctx context.Context, nc *nats.Conn, accountID string, body []byte, passRoleCheck PassRoleChecker) (any, error) { input := new(handlers_ecs.ProvisionCapacityInput) if err := unmarshalIfBody(body, input); err != nil { return nil, err @@ -105,7 +143,7 @@ func ProvisionCapacity(ctx context.Context, nc *nats.Conn, accountID string, bod return handlers_ecs.NewNATSECSService(nc).ProvisionCapacity(ctx, input, accountID) } -func DeregisterContainerInstance(ctx context.Context, nc *nats.Conn, accountID string, body []byte) (any, error) { +func DeregisterContainerInstance(ctx context.Context, nc *nats.Conn, accountID string, body []byte, passRoleCheck PassRoleChecker) (any, error) { input := new(ecs.DeregisterContainerInstanceInput) if err := unmarshalIfBody(body, input); err != nil { return nil, err @@ -113,7 +151,7 @@ func DeregisterContainerInstance(ctx context.Context, nc *nats.Conn, accountID s return handlers_ecs.NewNATSECSService(nc).DeregisterContainerInstance(ctx, input, accountID) } -func UpdateContainerInstancesState(ctx context.Context, nc *nats.Conn, accountID string, body []byte) (any, error) { +func UpdateContainerInstancesState(ctx context.Context, nc *nats.Conn, accountID string, body []byte, passRoleCheck PassRoleChecker) (any, error) { input := new(ecs.UpdateContainerInstancesStateInput) if err := unmarshalIfBody(body, input); err != nil { return nil, err @@ -121,7 +159,7 @@ func UpdateContainerInstancesState(ctx context.Context, nc *nats.Conn, accountID return handlers_ecs.NewNATSECSService(nc).UpdateContainerInstancesState(ctx, input, accountID) } -func DescribeContainerInstances(ctx context.Context, nc *nats.Conn, accountID string, body []byte) (any, error) { +func DescribeContainerInstances(ctx context.Context, nc *nats.Conn, accountID string, body []byte, passRoleCheck PassRoleChecker) (any, error) { input := new(ecs.DescribeContainerInstancesInput) if err := unmarshalIfBody(body, input); err != nil { return nil, err @@ -129,7 +167,7 @@ func DescribeContainerInstances(ctx context.Context, nc *nats.Conn, accountID st return handlers_ecs.NewNATSECSService(nc).DescribeContainerInstances(ctx, input, accountID) } -func ListContainerInstances(ctx context.Context, nc *nats.Conn, accountID string, body []byte) (any, error) { +func ListContainerInstances(ctx context.Context, nc *nats.Conn, accountID string, body []byte, passRoleCheck PassRoleChecker) (any, error) { input := new(ecs.ListContainerInstancesInput) if err := unmarshalIfBody(body, input); err != nil { return nil, err @@ -139,23 +177,41 @@ func ListContainerInstances(ctx context.Context, nc *nats.Conn, accountID string // --- Task --- -func RunTask(ctx context.Context, nc *nats.Conn, accountID string, body []byte) (any, error) { +func RunTask(ctx context.Context, nc *nats.Conn, accountID string, body []byte, passRoleCheck PassRoleChecker) (any, error) { input := new(ecs.RunTaskInput) if err := unmarshalIfBody(body, input); err != nil { return nil, err } - return handlers_ecs.NewNATSECSService(nc).RunTask(ctx, input, accountID) + return runTask(ctx, handlers_ecs.NewNATSECSService(nc), accountID, input, passRoleCheck) } -func StartTask(ctx context.Context, nc *nats.Conn, accountID string, body []byte) (any, error) { +// runTask is RunTask's core, split out so tests can inject a fake ECSService +// instead of a live NATS connection. +func runTask(ctx context.Context, svc handlers_ecs.ECSService, accountID string, input *ecs.RunTaskInput, passRoleCheck PassRoleChecker) (any, error) { + if err := checkTaskLaunchRoles(ctx, svc, accountID, input.TaskDefinition, passRoleCheck); err != nil { + return nil, err + } + return svc.RunTask(ctx, input, accountID) +} + +func StartTask(ctx context.Context, nc *nats.Conn, accountID string, body []byte, passRoleCheck PassRoleChecker) (any, error) { input := new(ecs.StartTaskInput) if err := unmarshalIfBody(body, input); err != nil { return nil, err } - return handlers_ecs.NewNATSECSService(nc).StartTask(ctx, input, accountID) + return startTask(ctx, handlers_ecs.NewNATSECSService(nc), accountID, input, passRoleCheck) } -func StopTask(ctx context.Context, nc *nats.Conn, accountID string, body []byte) (any, error) { +// startTask is StartTask's core, split out so tests can inject a fake +// ECSService instead of a live NATS connection. +func startTask(ctx context.Context, svc handlers_ecs.ECSService, accountID string, input *ecs.StartTaskInput, passRoleCheck PassRoleChecker) (any, error) { + if err := checkTaskLaunchRoles(ctx, svc, accountID, input.TaskDefinition, passRoleCheck); err != nil { + return nil, err + } + return svc.StartTask(ctx, input, accountID) +} + +func StopTask(ctx context.Context, nc *nats.Conn, accountID string, body []byte, passRoleCheck PassRoleChecker) (any, error) { input := new(ecs.StopTaskInput) if err := unmarshalIfBody(body, input); err != nil { return nil, err @@ -163,7 +219,7 @@ func StopTask(ctx context.Context, nc *nats.Conn, accountID string, body []byte) return handlers_ecs.NewNATSECSService(nc).StopTask(ctx, input, accountID) } -func DescribeTasks(ctx context.Context, nc *nats.Conn, accountID string, body []byte) (any, error) { +func DescribeTasks(ctx context.Context, nc *nats.Conn, accountID string, body []byte, passRoleCheck PassRoleChecker) (any, error) { input := new(ecs.DescribeTasksInput) if err := unmarshalIfBody(body, input); err != nil { return nil, err @@ -171,7 +227,7 @@ func DescribeTasks(ctx context.Context, nc *nats.Conn, accountID string, body [] return handlers_ecs.NewNATSECSService(nc).DescribeTasks(ctx, input, accountID) } -func ListTasks(ctx context.Context, nc *nats.Conn, accountID string, body []byte) (any, error) { +func ListTasks(ctx context.Context, nc *nats.Conn, accountID string, body []byte, passRoleCheck PassRoleChecker) (any, error) { input := new(ecs.ListTasksInput) if err := unmarshalIfBody(body, input); err != nil { return nil, err @@ -181,23 +237,45 @@ func ListTasks(ctx context.Context, nc *nats.Conn, accountID string, body []byte // --- Service --- -func CreateService(ctx context.Context, nc *nats.Conn, accountID string, body []byte) (any, error) { +// CreateService and UpdateService select a task definition and reconcile +// tasks under it entirely daemon-side, bypassing RunTask/StartTask, so +// iam:PassRole must be enforced here instead. +func CreateService(ctx context.Context, nc *nats.Conn, accountID string, body []byte, passRoleCheck PassRoleChecker) (any, error) { input := new(ecs.CreateServiceInput) if err := unmarshalIfBody(body, input); err != nil { return nil, err } - return handlers_ecs.NewNATSECSService(nc).CreateService(ctx, input, accountID) + return createService(ctx, handlers_ecs.NewNATSECSService(nc), accountID, input, passRoleCheck) } -func UpdateService(ctx context.Context, nc *nats.Conn, accountID string, body []byte) (any, error) { +// createService is CreateService's core, split out so tests can inject a fake +// ECSService instead of a live NATS connection. +func createService(ctx context.Context, svc handlers_ecs.ECSService, accountID string, input *ecs.CreateServiceInput, passRoleCheck PassRoleChecker) (any, error) { + if err := checkTaskLaunchRoles(ctx, svc, accountID, input.TaskDefinition, passRoleCheck); err != nil { + return nil, err + } + return svc.CreateService(ctx, input, accountID) +} + +func UpdateService(ctx context.Context, nc *nats.Conn, accountID string, body []byte, passRoleCheck PassRoleChecker) (any, error) { input := new(ecs.UpdateServiceInput) if err := unmarshalIfBody(body, input); err != nil { return nil, err } - return handlers_ecs.NewNATSECSService(nc).UpdateService(ctx, input, accountID) + return updateService(ctx, handlers_ecs.NewNATSECSService(nc), accountID, input, passRoleCheck) } -func DeleteService(ctx context.Context, nc *nats.Conn, accountID string, body []byte) (any, error) { +// updateService is UpdateService's core, split out so tests can inject a fake +// ECSService instead of a live NATS connection. A nil/empty TaskDefinition +// (desiredCount-only update) skips the check — nothing new to pass. +func updateService(ctx context.Context, svc handlers_ecs.ECSService, accountID string, input *ecs.UpdateServiceInput, passRoleCheck PassRoleChecker) (any, error) { + if err := checkTaskLaunchRoles(ctx, svc, accountID, input.TaskDefinition, passRoleCheck); err != nil { + return nil, err + } + return svc.UpdateService(ctx, input, accountID) +} + +func DeleteService(ctx context.Context, nc *nats.Conn, accountID string, body []byte, passRoleCheck PassRoleChecker) (any, error) { input := new(ecs.DeleteServiceInput) if err := unmarshalIfBody(body, input); err != nil { return nil, err @@ -205,7 +283,7 @@ func DeleteService(ctx context.Context, nc *nats.Conn, accountID string, body [] return handlers_ecs.NewNATSECSService(nc).DeleteService(ctx, input, accountID) } -func DescribeServices(ctx context.Context, nc *nats.Conn, accountID string, body []byte) (any, error) { +func DescribeServices(ctx context.Context, nc *nats.Conn, accountID string, body []byte, passRoleCheck PassRoleChecker) (any, error) { input := new(ecs.DescribeServicesInput) if err := unmarshalIfBody(body, input); err != nil { return nil, err @@ -213,7 +291,7 @@ func DescribeServices(ctx context.Context, nc *nats.Conn, accountID string, body return handlers_ecs.NewNATSECSService(nc).DescribeServices(ctx, input, accountID) } -func ListServices(ctx context.Context, nc *nats.Conn, accountID string, body []byte) (any, error) { +func ListServices(ctx context.Context, nc *nats.Conn, accountID string, body []byte, passRoleCheck PassRoleChecker) (any, error) { input := new(ecs.ListServicesInput) if err := unmarshalIfBody(body, input); err != nil { return nil, err @@ -224,7 +302,7 @@ func ListServices(ctx context.Context, nc *nats.Conn, accountID string, body []b // SubmitTaskStateChange is the agent's task-state report path over the gateway // (replaces the Layer-2 bus publish). The account is the SigV4 caller, so an // instance cannot report state for another account's task. -func SubmitTaskStateChange(ctx context.Context, nc *nats.Conn, accountID string, body []byte) (any, error) { +func SubmitTaskStateChange(ctx context.Context, nc *nats.Conn, accountID string, body []byte, passRoleCheck PassRoleChecker) (any, error) { input := new(ecs.SubmitTaskStateChangeInput) if err := unmarshalIfBody(body, input); err != nil { return nil, err @@ -236,7 +314,7 @@ func SubmitTaskStateChange(ctx context.Context, nc *nats.Conn, accountID string, // Layer-2 assign subscribe). Internal agent↔gateway action, not an AWS SDK // shape; the response carries bus.Assign with an RFC3339 time, so the gateway // encodes it with encoding/json (RawJSONActions), not the jsonutil marshaler. -func PollAssignments(ctx context.Context, nc *nats.Conn, accountID string, body []byte) (any, error) { +func PollAssignments(ctx context.Context, nc *nats.Conn, accountID string, body []byte, passRoleCheck PassRoleChecker) (any, error) { input := new(handlers_ecs.PollAssignmentsInput) if err := unmarshalIfBody(body, input); err != nil { return nil, err @@ -247,7 +325,7 @@ func PollAssignments(ctx context.Context, nc *nats.Conn, accountID string, body // ReportTaskGPU carries the agent's local ledger's per-task GPU device // assignment (agent → gateway). Internal action, not an AWS SDK shape: real // AWS's SubmitTaskStateChange has no gpuIds field. -func ReportTaskGPU(ctx context.Context, nc *nats.Conn, accountID string, body []byte) (any, error) { +func ReportTaskGPU(ctx context.Context, nc *nats.Conn, accountID string, body []byte, passRoleCheck PassRoleChecker) (any, error) { input := new(handlers_ecs.ReportTaskGPUInput) if err := unmarshalIfBody(body, input); err != nil { return nil, err @@ -257,7 +335,7 @@ func ReportTaskGPU(ctx context.Context, nc *nats.Conn, accountID string, body [] // --- Tags --- -func TagResource(ctx context.Context, nc *nats.Conn, accountID string, body []byte) (any, error) { +func TagResource(ctx context.Context, nc *nats.Conn, accountID string, body []byte, passRoleCheck PassRoleChecker) (any, error) { input := new(ecs.TagResourceInput) if err := unmarshalIfBody(body, input); err != nil { return nil, err @@ -265,7 +343,7 @@ func TagResource(ctx context.Context, nc *nats.Conn, accountID string, body []by return handlers_ecs.NewNATSECSService(nc).TagResource(ctx, input, accountID) } -func UntagResource(ctx context.Context, nc *nats.Conn, accountID string, body []byte) (any, error) { +func UntagResource(ctx context.Context, nc *nats.Conn, accountID string, body []byte, passRoleCheck PassRoleChecker) (any, error) { input := new(ecs.UntagResourceInput) if err := unmarshalIfBody(body, input); err != nil { return nil, err @@ -273,7 +351,7 @@ func UntagResource(ctx context.Context, nc *nats.Conn, accountID string, body [] return handlers_ecs.NewNATSECSService(nc).UntagResource(ctx, input, accountID) } -func ListTagsForResource(ctx context.Context, nc *nats.Conn, accountID string, body []byte) (any, error) { +func ListTagsForResource(ctx context.Context, nc *nats.Conn, accountID string, body []byte, passRoleCheck PassRoleChecker) (any, error) { input := new(ecs.ListTagsForResourceInput) if err := unmarshalIfBody(body, input); err != nil { return nil, err @@ -283,7 +361,7 @@ func ListTagsForResource(ctx context.Context, nc *nats.Conn, accountID string, b // --- Capacity providers --- -func PutClusterCapacityProviders(ctx context.Context, nc *nats.Conn, accountID string, body []byte) (any, error) { +func PutClusterCapacityProviders(ctx context.Context, nc *nats.Conn, accountID string, body []byte, passRoleCheck PassRoleChecker) (any, error) { input := new(ecs.PutClusterCapacityProvidersInput) if err := unmarshalIfBody(body, input); err != nil { return nil, err @@ -291,7 +369,7 @@ func PutClusterCapacityProviders(ctx context.Context, nc *nats.Conn, accountID s return handlers_ecs.NewNATSECSService(nc).PutClusterCapacityProviders(ctx, input, accountID) } -func CreateCapacityProvider(ctx context.Context, nc *nats.Conn, accountID string, body []byte) (any, error) { +func CreateCapacityProvider(ctx context.Context, nc *nats.Conn, accountID string, body []byte, passRoleCheck PassRoleChecker) (any, error) { input := new(ecs.CreateCapacityProviderInput) if err := unmarshalIfBody(body, input); err != nil { return nil, err @@ -299,7 +377,7 @@ func CreateCapacityProvider(ctx context.Context, nc *nats.Conn, accountID string return handlers_ecs.NewNATSECSService(nc).CreateCapacityProvider(ctx, input, accountID) } -func DescribeCapacityProviders(ctx context.Context, nc *nats.Conn, accountID string, body []byte) (any, error) { +func DescribeCapacityProviders(ctx context.Context, nc *nats.Conn, accountID string, body []byte, passRoleCheck PassRoleChecker) (any, error) { input := new(ecs.DescribeCapacityProvidersInput) if err := unmarshalIfBody(body, input); err != nil { return nil, err @@ -307,7 +385,7 @@ func DescribeCapacityProviders(ctx context.Context, nc *nats.Conn, accountID str return handlers_ecs.NewNATSECSService(nc).DescribeCapacityProviders(ctx, input, accountID) } -func DeleteCapacityProvider(ctx context.Context, nc *nats.Conn, accountID string, body []byte) (any, error) { +func DeleteCapacityProvider(ctx context.Context, nc *nats.Conn, accountID string, body []byte, passRoleCheck PassRoleChecker) (any, error) { input := new(ecs.DeleteCapacityProviderInput) if err := unmarshalIfBody(body, input); err != nil { return nil, err diff --git a/spinifex/gateway/ecs/actions_test.go b/spinifex/gateway/ecs/actions_test.go new file mode 100644 index 000000000..02375c2b7 --- /dev/null +++ b/spinifex/gateway/ecs/actions_test.go @@ -0,0 +1,274 @@ +package gateway_ecs + +import ( + "context" + "encoding/json" + "errors" + "testing" + + "github.com/aws/aws-sdk-go/aws" + "github.com/aws/aws-sdk-go/service/ecs" + "github.com/mulgadc/spinifex/spinifex/awserrors" + handlers_ecs "github.com/mulgadc/spinifex/spinifex/handlers/ecs" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +const testTaskRoleARN = "arn:aws:iam::123456789012:role/ecs-task-role" + +// fakeECSService is a minimal handlers_ecs.ECSService test double. Embedding +// the (nil) interface satisfies the contract; any method not overridden below +// nil-panics if called, surfacing an unexpected call the same way the EC2 +// gateway tests' fakeIAMService does. +type fakeECSService struct { + handlers_ecs.ECSService + + describeOut *ecs.DescribeTaskDefinitionOutput + describeErr error + + runOut *ecs.RunTaskOutput + runErr error + runCalled bool + + startOut *ecs.StartTaskOutput + startErr error + startCalled bool + + createOut *ecs.CreateServiceOutput + createErr error + createCalled bool + + updateOut *ecs.UpdateServiceOutput + updateErr error + updateCalled bool +} + +func (f *fakeECSService) DescribeTaskDefinition(_ context.Context, _ *ecs.DescribeTaskDefinitionInput, _ string) (*ecs.DescribeTaskDefinitionOutput, error) { + return f.describeOut, f.describeErr +} + +func (f *fakeECSService) RunTask(_ context.Context, _ *ecs.RunTaskInput, _ string) (*ecs.RunTaskOutput, error) { + f.runCalled = true + return f.runOut, f.runErr +} + +func (f *fakeECSService) StartTask(_ context.Context, _ *ecs.StartTaskInput, _ string) (*ecs.StartTaskOutput, error) { + f.startCalled = true + return f.startOut, f.startErr +} + +func (f *fakeECSService) CreateService(_ context.Context, _ *ecs.CreateServiceInput, _ string) (*ecs.CreateServiceOutput, error) { + f.createCalled = true + return f.createOut, f.createErr +} + +func (f *fakeECSService) UpdateService(_ context.Context, _ *ecs.UpdateServiceInput, _ string) (*ecs.UpdateServiceOutput, error) { + f.updateCalled = true + return f.updateOut, f.updateErr +} + +// taskDefWithRole is a DescribeTaskDefinition response naming testTaskRoleARN +// as both the task role and the execution role. +func taskDefWithRole() *ecs.DescribeTaskDefinitionOutput { + return &ecs.DescribeTaskDefinitionOutput{ + TaskDefinition: &ecs.TaskDefinition{ + TaskRoleArn: aws.String(testTaskRoleARN), + ExecutionRoleArn: aws.String(testTaskRoleARN), + }, + } +} + +// taskDefWithoutRole is a DescribeTaskDefinition response naming no role — +// the compatibility case that must keep working unchanged. +func taskDefWithoutRole() *ecs.DescribeTaskDefinitionOutput { + return &ecs.DescribeTaskDefinitionOutput{TaskDefinition: &ecs.TaskDefinition{}} +} + +func denyCheck(t *testing.T, wantARN string) PassRoleChecker { + t.Helper() + return func(roleARN string) error { + assert.Equal(t, wantARN, roleARN) + return errors.New(awserrors.ErrorAccessDenied) + } +} + +func allowCheck(t *testing.T, wantARN string) PassRoleChecker { + t.Helper() + return func(roleARN string) error { + assert.Equal(t, wantARN, roleARN) + return nil + } +} + +func mustNotBeCalledCheck(t *testing.T) PassRoleChecker { + t.Helper() + return func(roleARN string) error { + t.Errorf("PassRole must not be checked when no role is named (got %q)", roleARN) + return nil + } +} + +// --- checkTaskDefinitionRoles / checkTaskLaunchRoles ------------------------ + +func TestCheckTaskDefinitionRoles_NilCheckerSkips(t *testing.T) { + assert.NoError(t, checkTaskDefinitionRoles(nil, aws.String(testTaskRoleARN))) +} + +func TestCheckTaskDefinitionRoles_EmptyARNSkipped(t *testing.T) { + assert.NoError(t, checkTaskDefinitionRoles(mustNotBeCalledCheck(t), aws.String(""), nil)) +} + +func TestCheckTaskDefinitionRoles_Denied(t *testing.T) { + err := checkTaskDefinitionRoles(denyCheck(t, testTaskRoleARN), aws.String(testTaskRoleARN)) + require.Error(t, err) + assert.Equal(t, awserrors.ErrorAccessDenied, err.Error()) +} + +func TestCheckTaskDefinitionRoles_Allowed(t *testing.T) { + assert.NoError(t, checkTaskDefinitionRoles(allowCheck(t, testTaskRoleARN), aws.String(testTaskRoleARN))) +} + +func TestCheckTaskLaunchRoles_NilCheckerSkipsDescribe(t *testing.T) { + svc := &fakeECSService{describeErr: errors.New("must not be called")} + assert.NoError(t, checkTaskLaunchRoles(context.Background(), svc, "123456789012", aws.String("family:1"), nil)) +} + +func TestCheckTaskLaunchRoles_DescribeErrorPropagates(t *testing.T) { + svc := &fakeECSService{describeErr: errors.New(awserrors.ErrorECSInvalidParameter)} + err := checkTaskLaunchRoles(context.Background(), svc, "123456789012", aws.String("ghost:1"), allowCheck(t, testTaskRoleARN)) + require.Error(t, err) + assert.Equal(t, awserrors.ErrorECSInvalidParameter, err.Error()) +} + +// --- RegisterTaskDefinition -------------------------------------------------- + +func registerTaskDefBody(t *testing.T, taskRoleARN string) []byte { + t.Helper() + input := &ecs.RegisterTaskDefinitionInput{Family: aws.String("web")} + if taskRoleARN != "" { + input.TaskRoleArn = aws.String(taskRoleARN) + } + body, err := json.Marshal(input) + require.NoError(t, err) + return body +} + +func TestRegisterTaskDefinition_DeniedWithoutPassRole(t *testing.T) { + _, err := RegisterTaskDefinition(context.Background(), nil, "123456789012", + registerTaskDefBody(t, testTaskRoleARN), denyCheck(t, testTaskRoleARN)) + require.Error(t, err) + assert.Equal(t, awserrors.ErrorAccessDenied, err.Error()) +} + +func TestRegisterTaskDefinition_AllowedWithPassRole(t *testing.T) { + // nc is nil, so once the PassRole check passes the NATS dispatch fails — + // any error here must not be AccessDenied, proving the check let it through. + _, err := RegisterTaskDefinition(context.Background(), nil, "123456789012", + registerTaskDefBody(t, testTaskRoleARN), allowCheck(t, testTaskRoleARN)) + require.Error(t, err) + assert.NotEqual(t, awserrors.ErrorAccessDenied, err.Error()) +} + +func TestRegisterTaskDefinition_NoRoleSkipsPassRole(t *testing.T) { + _, err := RegisterTaskDefinition(context.Background(), nil, "123456789012", + registerTaskDefBody(t, ""), mustNotBeCalledCheck(t)) + require.Error(t, err) // still fails downstream (nil NATS conn), just not on PassRole + assert.NotEqual(t, awserrors.ErrorAccessDenied, err.Error()) +} + +// --- runTask / startTask ----------------------------------------------------- + +func TestRunTask_DeniedWithoutPassRole(t *testing.T) { + svc := &fakeECSService{describeOut: taskDefWithRole()} + _, err := runTask(context.Background(), svc, "123456789012", + &ecs.RunTaskInput{TaskDefinition: aws.String("web:1")}, denyCheck(t, testTaskRoleARN)) + require.Error(t, err) + assert.Equal(t, awserrors.ErrorAccessDenied, err.Error()) + assert.False(t, svc.runCalled, "RunTask must not dispatch when PassRole is denied") +} + +func TestRunTask_AllowedWithPassRole(t *testing.T) { + svc := &fakeECSService{describeOut: taskDefWithRole(), runOut: &ecs.RunTaskOutput{}} + _, err := runTask(context.Background(), svc, "123456789012", + &ecs.RunTaskInput{TaskDefinition: aws.String("web:1")}, allowCheck(t, testTaskRoleARN)) + require.NoError(t, err) + assert.True(t, svc.runCalled, "RunTask must dispatch once PassRole is granted") +} + +func TestRunTask_NoRoleSkipsPassRole(t *testing.T) { + svc := &fakeECSService{describeOut: taskDefWithoutRole(), runOut: &ecs.RunTaskOutput{}} + _, err := runTask(context.Background(), svc, "123456789012", + &ecs.RunTaskInput{TaskDefinition: aws.String("web:1")}, mustNotBeCalledCheck(t)) + require.NoError(t, err) + assert.True(t, svc.runCalled, "a roleless task definition must still launch") +} + +func TestStartTask_DeniedWithoutPassRole(t *testing.T) { + svc := &fakeECSService{describeOut: taskDefWithRole()} + _, err := startTask(context.Background(), svc, "123456789012", + &ecs.StartTaskInput{TaskDefinition: aws.String("web:1")}, denyCheck(t, testTaskRoleARN)) + require.Error(t, err) + assert.Equal(t, awserrors.ErrorAccessDenied, err.Error()) + assert.False(t, svc.startCalled, "StartTask must not dispatch when PassRole is denied") +} + +func TestStartTask_AllowedWithPassRole(t *testing.T) { + svc := &fakeECSService{describeOut: taskDefWithRole(), startOut: &ecs.StartTaskOutput{}} + _, err := startTask(context.Background(), svc, "123456789012", + &ecs.StartTaskInput{TaskDefinition: aws.String("web:1")}, allowCheck(t, testTaskRoleARN)) + require.NoError(t, err) + assert.True(t, svc.startCalled, "StartTask must dispatch once PassRole is granted") +} + +func TestStartTask_NoRoleSkipsPassRole(t *testing.T) { + svc := &fakeECSService{describeOut: taskDefWithoutRole(), startOut: &ecs.StartTaskOutput{}} + _, err := startTask(context.Background(), svc, "123456789012", + &ecs.StartTaskInput{TaskDefinition: aws.String("web:1")}, mustNotBeCalledCheck(t)) + require.NoError(t, err) + assert.True(t, svc.startCalled, "a roleless task definition must still launch") +} + +// --- createService / updateService ------------------------------------------ + +func TestCreateService_DeniedWithoutPassRole(t *testing.T) { + svc := &fakeECSService{describeOut: taskDefWithRole()} + _, err := createService(context.Background(), svc, "123456789012", + &ecs.CreateServiceInput{TaskDefinition: aws.String("web:1")}, denyCheck(t, testTaskRoleARN)) + require.Error(t, err) + assert.Equal(t, awserrors.ErrorAccessDenied, err.Error()) + assert.False(t, svc.createCalled, "CreateService must not dispatch when PassRole is denied") +} + +func TestCreateService_AllowedWithPassRole(t *testing.T) { + svc := &fakeECSService{describeOut: taskDefWithRole(), createOut: &ecs.CreateServiceOutput{}} + _, err := createService(context.Background(), svc, "123456789012", + &ecs.CreateServiceInput{TaskDefinition: aws.String("web:1")}, allowCheck(t, testTaskRoleARN)) + require.NoError(t, err) + assert.True(t, svc.createCalled) +} + +func TestUpdateService_DeniedWithoutPassRole(t *testing.T) { + svc := &fakeECSService{describeOut: taskDefWithRole()} + _, err := updateService(context.Background(), svc, "123456789012", + &ecs.UpdateServiceInput{TaskDefinition: aws.String("web:2")}, denyCheck(t, testTaskRoleARN)) + require.Error(t, err) + assert.Equal(t, awserrors.ErrorAccessDenied, err.Error()) + assert.False(t, svc.updateCalled, "UpdateService must not dispatch when PassRole is denied") +} + +func TestUpdateService_AllowedWithPassRole(t *testing.T) { + svc := &fakeECSService{describeOut: taskDefWithRole(), updateOut: &ecs.UpdateServiceOutput{}} + _, err := updateService(context.Background(), svc, "123456789012", + &ecs.UpdateServiceInput{TaskDefinition: aws.String("web:2")}, allowCheck(t, testTaskRoleARN)) + require.NoError(t, err) + assert.True(t, svc.updateCalled) +} + +func TestUpdateService_NoTaskDefinitionSkipsPassRole(t *testing.T) { + // A desiredCount-only update names no task definition at all. + svc := &fakeECSService{updateOut: &ecs.UpdateServiceOutput{}} + _, err := updateService(context.Background(), svc, "123456789012", + &ecs.UpdateServiceInput{DesiredCount: aws.Int64(3)}, mustNotBeCalledCheck(t)) + require.NoError(t, err) + assert.True(t, svc.updateCalled) +} diff --git a/spinifex/gateway/ecs/handler.go b/spinifex/gateway/ecs/handler.go index a63fe0682..31604b51d 100644 --- a/spinifex/gateway/ecs/handler.go +++ b/spinifex/gateway/ecs/handler.go @@ -26,15 +26,19 @@ const TargetPrefix = "AmazonEC2ContainerServiceV20141113" // JSONContentType is the AWS JSON 1.1 content type ECS clients expect. const JSONContentType = "application/x-amz-json-1.1" -// Handler is the signature every ECS control-plane action implements. nc is the -// gateway NATS connection (handlers relay onto ecs.* subjects); accountID is the -// resolved caller account; body is the raw JSON 1.1 request payload. -type Handler func(ctx context.Context, nc *nats.Conn, accountID string, body []byte) (any, error) +// PassRoleChecker enforces iam:PassRole on a role ARN a task definition names, +// built from the caller's identity and threaded down from the gateway. +type PassRoleChecker func(roleARN string) error + +// Handler is the signature every ECS control-plane action implements. nc, accountID +// and body carry the request; passRoleCheck enforces iam:PassRole for actions that +// can name an IAM role. +type Handler func(ctx context.Context, nc *nats.Conn, accountID string, body []byte, passRoleCheck PassRoleChecker) (any, error) // NotImplemented is the placeholder for every unimplemented ECS control-plane // action. It returns the AWS NotImplemented error, which the shared gateway // ErrorHandler renders as a 501 JSON 1.1 envelope. -func NotImplemented(_ context.Context, _ *nats.Conn, _ string, _ []byte) (any, error) { +func NotImplemented(_ context.Context, _ *nats.Conn, _ string, _ []byte, _ PassRoleChecker) (any, error) { return nil, errors.New(awserrors.ErrorNotImplemented) } diff --git a/spinifex/gateway/ecs/handler_test.go b/spinifex/gateway/ecs/handler_test.go index 7ba9289b2..98274f2c4 100644 --- a/spinifex/gateway/ecs/handler_test.go +++ b/spinifex/gateway/ecs/handler_test.go @@ -12,7 +12,7 @@ import ( ) func TestNotImplemented(t *testing.T) { - out, err := NotImplemented(context.Background(), nil, "123456789012", []byte("{}")) + out, err := NotImplemented(context.Background(), nil, "123456789012", []byte("{}"), nil) assert.Nil(t, out) require.Error(t, err) assert.Equal(t, awserrors.ErrorNotImplemented, err.Error()) @@ -42,7 +42,7 @@ var wiredActions = map[string]bool{ func TestActions_StubsReturnNotImplemented(t *testing.T) { require.NotEmpty(t, Actions) for action, h := range Actions { - _, err := h(context.Background(), nil, "123456789012", []byte("{}")) + _, err := h(context.Background(), nil, "123456789012", []byte("{}"), nil) require.Error(t, err, "action %q", action) if wiredActions[action] { assert.NotEqual(t, awserrors.ErrorNotImplemented, err.Error(), "wired action %q should not be a stub", action) diff --git a/spinifex/gateway/ecs_passrole_test.go b/spinifex/gateway/ecs_passrole_test.go new file mode 100644 index 000000000..55cb3cd53 --- /dev/null +++ b/spinifex/gateway/ecs_passrole_test.go @@ -0,0 +1,137 @@ +package gateway + +import ( + "context" + "encoding/json" + "net/http" + "net/http/httptest" + "testing" + + "github.com/aws/aws-sdk-go/aws" + "github.com/aws/aws-sdk-go/service/ecs" + "github.com/mulgadc/spinifex/spinifex/awserrors" + handlers_iam "github.com/mulgadc/spinifex/spinifex/handlers/iam" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +// TestECSRegisterTaskDefinition_PassRole exercises the full gw.ECS_Request +// dispatch (X-Amz-Target parsing, action-level checkPolicy, then the +// iam:PassRole resource check gateway/ecs.go builds and threads through +// gateway_ecs.Handler) with a real authenticated user identity, so the IAM +// policy is actually evaluated rather than taking the pre-IAM bypass. +const testECSPassRoleTargetARN = "arn:aws:iam::123456789012:role/ecs-task-role" + +// setupECSUserRequest is setupECSRequest plus an authenticated user identity. +func setupECSUserRequest(target, body string) *http.Request { + req := setupECSRequest(target, body) + ctx := context.WithValue(req.Context(), ctxIdentity, "alice") + ctx = context.WithValue(ctx, ctxPrincipalType, principalTypeUser) + return req.WithContext(ctx) +} + +func registerTaskDefinitionBody(t *testing.T, taskRoleARN string) string { + t.Helper() + input := &ecs.RegisterTaskDefinitionInput{Family: aws.String("web")} + if taskRoleARN != "" { + input.TaskRoleArn = aws.String(taskRoleARN) + } + body, err := json.Marshal(input) + require.NoError(t, err) + return string(body) +} + +// allowRegisterTaskDefAndPassRole grants ecs:RegisterTaskDefinition +// unconditionally (the action-level check gateway/ecs.go:39 always runs +// first) plus iam:PassRole scoped to passRoleResource, so tests can isolate +// the resource-level PassRole decision from the action-level one. +func allowRegisterTaskDefAndPassRole(passRoleResource string) []handlers_iam.PolicyDocument { + return []handlers_iam.PolicyDocument{{ + Version: "2012-10-17", + Statement: []handlers_iam.Statement{ + {Effect: "Allow", Action: handlers_iam.StringOrArr{"ecs:RegisterTaskDefinition"}, Resource: handlers_iam.StringOrArr{"*"}}, + {Effect: "Allow", Action: handlers_iam.StringOrArr{"iam:PassRole"}, Resource: handlers_iam.StringOrArr{passRoleResource}}, + }, + }} +} + +func TestECSRegisterTaskDefinition_DeniedWithoutPassRole(t *testing.T) { + mock := &policyMockIAMService{ + getUserPoliciesFn: func(_, _ string) ([]handlers_iam.PolicyDocument, error) { + // ecs:RegisterTaskDefinition is granted; iam:PassRole is scoped to a + // different role, so the target ARN is specifically denied. + return allowRegisterTaskDefAndPassRole("arn:aws:iam::123456789012:role/some-other-role"), nil + }, + } + gw := &GatewayConfig{DisableLogging: true, IAMService: mock} + req := setupECSUserRequest("AmazonEC2ContainerServiceV20141113.RegisterTaskDefinition", + registerTaskDefinitionBody(t, testECSPassRoleTargetARN)) + + err := gw.ECS_Request(httptest.NewRecorder(), req) + require.Error(t, err) + assert.Equal(t, awserrors.ErrorAccessDenied, err.Error()) +} + +func TestECSRegisterTaskDefinition_AllowedWithPassRole(t *testing.T) { + mock := &policyMockIAMService{ + getUserPoliciesFn: func(_, _ string) ([]handlers_iam.PolicyDocument, error) { + return allowRegisterTaskDefAndPassRole(testECSPassRoleTargetARN), nil + }, + } + gw := &GatewayConfig{DisableLogging: true, IAMService: mock} + req := setupECSUserRequest("AmazonEC2ContainerServiceV20141113.RegisterTaskDefinition", + registerTaskDefinitionBody(t, testECSPassRoleTargetARN)) + + err := gw.ECS_Request(httptest.NewRecorder(), req) + // PassRole is granted, so the request proceeds past the check to the (nil, + // disconnected) NATS dispatch; whatever fails downstream, it must not be + // the PassRole denial. + require.Error(t, err) + assert.NotEqual(t, awserrors.ErrorAccessDenied, err.Error()) +} + +func TestECSRegisterTaskDefinition_NoRoleSkipsPassRole(t *testing.T) { + mock := &policyMockIAMService{ + getUserPoliciesFn: func(_, _ string) ([]handlers_iam.PolicyDocument, error) { + // No iam:PassRole grant at all — if the gateway still consulted + // PassRole for a roleless task definition this would deny it. + return allowPolicyResource("ecs:RegisterTaskDefinition", "*"), nil + }, + } + gw := &GatewayConfig{DisableLogging: true, IAMService: mock} + req := setupECSUserRequest("AmazonEC2ContainerServiceV20141113.RegisterTaskDefinition", + registerTaskDefinitionBody(t, "")) + + err := gw.ECS_Request(httptest.NewRecorder(), req) + require.Error(t, err) // still fails downstream (nil NATS conn), just not on PassRole + assert.NotEqual(t, awserrors.ErrorAccessDenied, err.Error()) +} + +// TestECSRegisterTaskDefinition_DenialRendersAccessDenied403 proves the denial +// reaches the wire as AWS's real PassRole-denial shape: HTTP 403, AWS JSON 1.1 +// envelope, the same AccessDenied code/message the EC2 iam:PassRole path uses. +func TestECSRegisterTaskDefinition_DenialRendersAccessDenied403(t *testing.T) { + mock := &policyMockIAMService{ + getUserPoliciesFn: func(_, _ string) ([]handlers_iam.PolicyDocument, error) { + return allowPolicyResource("ecs:RegisterTaskDefinition", "*"), nil + }, + } + gw := &GatewayConfig{DisableLogging: true, IAMService: mock} + req := setupECSUserRequest("AmazonEC2ContainerServiceV20141113.RegisterTaskDefinition", + registerTaskDefinitionBody(t, testECSPassRoleTargetARN)) + + w := httptest.NewRecorder() + err := gw.ECS_Request(w, req) + require.Error(t, err) + + gw.ErrorHandler(w, req, err) + assert.Equal(t, http.StatusForbidden, w.Code) + + var body struct { + Type string `json:"__type"` + Message string `json:"message"` + } + require.NoError(t, json.Unmarshal(w.Body.Bytes(), &body)) + assert.Equal(t, "AccessDeniedException", body.Type) + assert.Equal(t, "User is not authorized to perform this action.", body.Message) +}