From 4869f3b021d152dcb7e8cb21919438e2a1f812cf Mon Sep 17 00:00:00 2001 From: Julian Sommer <115516918+juliansommer@users.noreply.github.com> Date: Mon, 24 Aug 2026 15:49:07 +1000 Subject: [PATCH] fix: evaluate EC2 policies against the request's resource ARNs (#866) --- spinifex/awsec2query/parser.go | 10 +- spinifex/awsec2query/parser_test.go | 2 +- spinifex/gateway/ec2.go | 157 ++++--- spinifex/gateway/ec2/authz.go | 430 +++++++++++++++++ spinifex/gateway/ec2/authz_test.go | 440 ++++++++++++++++++ spinifex/gateway/ec2/instance/RunInstances.go | 6 +- .../gateway/ec2_authz_completeness_test.go | 27 ++ spinifex/gateway/ec2_authz_test.go | 244 ++++++++++ spinifex/gateway/ec2_emptyresult_test.go | 5 +- spinifex/gateway/gateway.go | 5 +- spinifex/gateway/gateway_test.go | 5 +- spinifex/gateway/policy/evaluator_test.go | 65 +++ 12 files changed, 1328 insertions(+), 68 deletions(-) create mode 100644 spinifex/gateway/ec2/authz.go create mode 100644 spinifex/gateway/ec2/authz_test.go create mode 100644 spinifex/gateway/ec2_authz_completeness_test.go create mode 100644 spinifex/gateway/ec2_authz_test.go diff --git a/spinifex/awsec2query/parser.go b/spinifex/awsec2query/parser.go index b1c7fb4aa..95f3be56e 100644 --- a/spinifex/awsec2query/parser.go +++ b/spinifex/awsec2query/parser.go @@ -177,9 +177,9 @@ func setFieldValue(field reflect.Value, value string) error { return nil } -// maxSliceLen caps list materialisation to prevent unbounded allocation -// from adversarial indexes like `Filter.999999`. -const maxSliceLen = 1024 +// MaxSliceLen caps list materialisation to prevent unbounded allocation from +// adversarial indexes like `Filter.999999`. +const MaxSliceLen = 1024 // Records the 1-based indices of the entries stored under prefix, reporting // whether it matched anything at all. @@ -227,8 +227,8 @@ func setSliceField(field reflect.Value, params map[string]string, prefix, listNa denseLen := 0 for indices[denseLen+1] { denseLen++ - if denseLen > maxSliceLen { - return fmt.Errorf("%w: %q (max %d)", ErrSliceTooLarge, prefix, maxSliceLen) + if denseLen > MaxSliceLen { + return fmt.Errorf("%w: %q (max %d)", ErrSliceTooLarge, prefix, MaxSliceLen) } } if denseLen == 0 { diff --git a/spinifex/awsec2query/parser_test.go b/spinifex/awsec2query/parser_test.go index 5e2dde8ff..a4f4d74bc 100644 --- a/spinifex/awsec2query/parser_test.go +++ b/spinifex/awsec2query/parser_test.go @@ -663,7 +663,7 @@ func TestQueryParamsToStruct_GapAtIndexOne(t *testing.T) { func TestQueryParamsToStruct_RejectsOversizedDenseList(t *testing.T) { args := map[string]string{"Action": "DescribeInstances"} - for i := 1; i <= maxSliceLen+1; i++ { + for i := 1; i <= MaxSliceLen+1; i++ { args[fmt.Sprintf("Filter.%d.Name", i)] = "x" args[fmt.Sprintf("Filter.%d.Value.1", i)] = "y" } diff --git a/spinifex/gateway/ec2.go b/spinifex/gateway/ec2.go index 2f2189064..835530c4f 100644 --- a/spinifex/gateway/ec2.go +++ b/spinifex/gateway/ec2.go @@ -13,6 +13,7 @@ import ( "github.com/aws/aws-sdk-go/service/ec2" "github.com/mulgadc/spinifex/spinifex/awsec2query" "github.com/mulgadc/spinifex/spinifex/awserrors" + gateway_ec2 "github.com/mulgadc/spinifex/spinifex/gateway/ec2" gateway_ec2_account "github.com/mulgadc/spinifex/spinifex/gateway/ec2/account" gateway_ec2_capacityreservation "github.com/mulgadc/spinifex/spinifex/gateway/ec2/capacityreservation" gateway_ec2_eigw "github.com/mulgadc/spinifex/spinifex/gateway/ec2/eigw" @@ -36,9 +37,23 @@ import ( ) // EC2Handler processes parsed query args and returns XML response bytes. -// r is included for handlers that call gw.checkPolicyResource (e.g. iam:PassRole); -// most handlers ignore it. -type EC2Handler func(action string, q map[string]string, gw *GatewayConfig, accountID string, r *http.Request) ([]byte, error) +// r is included for handlers that enforce iam:PassRole; most ignore it. +type EC2Handler func(string, map[string]string, *GatewayConfig, string, *http.Request) ([]byte, error) + +// ec2Action parses once so authorization and dispatch share one typed input. +type ec2Action struct { + parse func(map[string]string) (any, error) + dispatch func(string, any, *GatewayConfig, string, *http.Request) ([]byte, error) +} + +func (h ec2Action) withQueryPreprocessor(preprocess func(map[string]string)) ec2Action { + parse := h.parse + h.parse = func(q map[string]string) (any, error) { + preprocess(q) + return parse(q) + } + return h +} // requestContext returns r's context, or Background for a nil request // (some callers and tests dispatch without an *http.Request). The SDK's @@ -52,26 +67,32 @@ func requestContext(r *http.Request) context.Context { return utils.WithIdempotencyKey(r.Context(), r.Header.Get(utils.SDKInvocationIDHeader)) } -// ec2Handler creates a type-safe EC2Handler: allocates the input struct, -// parses query params, calls the handler, and marshals output to XML. -func ec2Handler[In any](handler func(ctx context.Context, input *In, gw *GatewayConfig, accountID string) (any, error)) EC2Handler { - return func(action string, q map[string]string, gw *GatewayConfig, accountID string, r *http.Request) ([]byte, error) { - input := new(In) - if err := awsec2query.QueryParamsToStruct(q, input); err != nil { - if errors.Is(err, awsec2query.ErrSliceTooLarge) { - return nil, errors.New(awserrors.ErrorMalformedQueryString) +// ec2Handler creates a type-safe EC2Handler. Parsing is separate from dispatch +// so authorization and execution consume the exact same input. +func ec2Handler[In any](handler func(ctx context.Context, input *In, gw *GatewayConfig, accountID string) (any, error)) ec2Action { + return ec2Action{ + parse: func(q map[string]string) (any, error) { + input := new(In) + if err := awsec2query.QueryParamsToStruct(q, input); err != nil { + return nil, err } - return nil, err - } - output, err := handler(requestContext(r), input, gw, accountID) - if err != nil { - return nil, err - } - xmlOutput, err := marshalEC2Response(action, output) - if err != nil { - return nil, err - } - return xmlOutput, nil + return input, nil + }, + dispatch: func(action string, parsed any, gw *GatewayConfig, accountID string, r *http.Request) ([]byte, error) { + input, ok := parsed.(*In) + if !ok { + return nil, errors.New(awserrors.ErrorInternalError) + } + output, err := handler(requestContext(r), input, gw, accountID) + if err != nil { + return nil, err + } + xmlOutput, err := marshalEC2Response(action, output) + if err != nil { + return nil, err + } + return xmlOutput, nil + }, } } @@ -94,28 +115,34 @@ func marshalEC2Response(action string, output any) ([]byte, error) { // ec2HandlerWithReq is ec2Handler for actions that need the original *http.Request, // e.g. RunInstances which enforces iam:PassRole on the supplied instance profile ARN. -func ec2HandlerWithReq[In any](handler func(ctx context.Context, input *In, gw *GatewayConfig, accountID string, r *http.Request) (any, error)) EC2Handler { - return func(action string, q map[string]string, gw *GatewayConfig, accountID string, r *http.Request) ([]byte, error) { - input := new(In) - if err := awsec2query.QueryParamsToStruct(q, input); err != nil { - if errors.Is(err, awsec2query.ErrSliceTooLarge) { - return nil, errors.New(awserrors.ErrorMalformedQueryString) +func ec2HandlerWithReq[In any](handler func(ctx context.Context, input *In, gw *GatewayConfig, accountID string, r *http.Request) (any, error)) ec2Action { + return ec2Action{ + parse: func(q map[string]string) (any, error) { + input := new(In) + if err := awsec2query.QueryParamsToStruct(q, input); err != nil { + return nil, err } - return nil, err - } - output, err := handler(requestContext(r), input, gw, accountID, r) - if err != nil { - return nil, err - } - xmlOutput, err := marshalEC2Response(action, output) - if err != nil { - return nil, err - } - return xmlOutput, nil + return input, nil + }, + dispatch: func(action string, parsed any, gw *GatewayConfig, accountID string, r *http.Request) ([]byte, error) { + input, ok := parsed.(*In) + if !ok { + return nil, errors.New(awserrors.ErrorInternalError) + } + output, err := handler(requestContext(r), input, gw, accountID, r) + if err != nil { + return nil, err + } + xmlOutput, err := marshalEC2Response(action, output) + if err != nil { + return nil, err + } + return xmlOutput, nil + }, } } -var ec2Actions = map[string]EC2Handler{ +var ec2Actions = map[string]ec2Action{ "DescribeInstances": ec2Handler(func(ctx context.Context, input *ec2.DescribeInstancesInput, gw *GatewayConfig, accountID string) (any, error) { out, err := gateway_ec2_instance.DescribeInstancesChecked(ctx, input, gw.NATSConn, gw.DiscoverActiveNodes(ctx), accountID) if err != nil { @@ -229,15 +256,14 @@ var ec2Actions = map[string]EC2Handler{ "DescribeKeyPairs": ec2Handler(func(ctx context.Context, input *ec2.DescribeKeyPairsInput, gw *GatewayConfig, accountID string) (any, error) { return gateway_ec2_key.DescribeKeyPairs(ctx, input, gw.NATSConn, accountID) }), - "ImportKeyPair": func(action string, q map[string]string, gw *GatewayConfig, accountID string, r *http.Request) ([]byte, error) { + "ImportKeyPair": ec2Handler(func(ctx context.Context, input *ec2.ImportKeyPairInput, gw *GatewayConfig, accountID string) (any, error) { + return gateway_ec2_key.ImportKeyPair(ctx, input, gw.NATSConn, accountID) + }).withQueryPreprocessor(func(q map[string]string) { // Parser leaves Base64 padding URL-encoded; decode it before dispatch. if strings.HasSuffix(q["PublicKeyMaterial"], "%3D%3D") { q["PublicKeyMaterial"] = strings.Replace(q["PublicKeyMaterial"], "%3D%3D", "==", 1) } - return ec2Handler(func(ctx context.Context, input *ec2.ImportKeyPairInput, gw *GatewayConfig, accountID string) (any, error) { - return gateway_ec2_key.ImportKeyPair(ctx, input, gw.NATSConn, accountID) - })(action, q, gw, accountID, r) - }, + }), "DescribeImages": ec2Handler(func(ctx context.Context, input *ec2.DescribeImagesInput, gw *GatewayConfig, accountID string) (any, error) { return gateway_ec2_image.DescribeImages(ctx, input, gw.NATSConn, accountID) }), @@ -572,7 +598,38 @@ func (gw *GatewayConfig) EC2_Request(w http.ResponseWriter, r *http.Request) err return errors.New(awserrors.ErrorInvalidAction) } - if err := gw.checkPolicy(r, "ec2", action); err != nil { + // Hoisted above the policy check because the resolver builds ARNs from it. + // gw.Region, never the caller-supplied credential-scope region, which would + // let a caller sign for another region and slide out from under a Deny. + accountID, _ := r.Context().Value(ctxAccountID).(string) + if accountID == "" { + slog.Error("EC2_Request: no account ID in auth context") + // InternalError, not ServerInternal: the policy gate used to reach this + // case first and that is the code the caller has always seen. + return errors.New(awserrors.ErrorInternalError) + } + + input, err := handler.parse(queryArgs) + if err != nil { + if errors.Is(err, awsec2query.ErrSliceTooLarge) { + return errors.New(awserrors.ErrorMalformedQueryString) + } + return err + } + + // Launch templates supply effective RunInstances resources. Resolve them + // before authorization, and dispatch the same expanded input afterwards. + if runInput, ok := input.(*ec2.RunInstancesInput); ok && action == "RunInstances" { + if err := gateway_ec2_instance.ExpandLaunchTemplate(r.Context(), gw.NATSConn, runInput, accountID); err != nil { + return err + } + } + + resources, err := gateway_ec2.ResourceARNs(action, gw.Region, accountID, input) + if err != nil { + return err + } + if err := gw.checkPolicyResources(r, "ec2", action, resources); err != nil { return err } @@ -580,13 +637,7 @@ func (gw *GatewayConfig) EC2_Request(w http.ResponseWriter, r *http.Request) err return errors.New(awserrors.ErrorServerInternal) } - accountID, _ := r.Context().Value(ctxAccountID).(string) - if accountID == "" { - slog.Error("EC2_Request: no account ID in auth context") - return errors.New(awserrors.ErrorServerInternal) - } - - xmlOutput, err := handler(action, queryArgs, gw, accountID, r) + xmlOutput, err := handler.dispatch(action, input, gw, accountID, r) if err != nil { return err } diff --git a/spinifex/gateway/ec2/authz.go b/spinifex/gateway/ec2/authz.go new file mode 100644 index 000000000..bbf953271 --- /dev/null +++ b/spinifex/gateway/ec2/authz.go @@ -0,0 +1,430 @@ +// Package gateway_ec2 resolves the resource ARNs an EC2 request authorizes +// against. Without it every EC2 action is evaluated against the literal "*", so +// a resource-scoped Deny never fires and a resource-scoped Allow never grants. +package gateway_ec2 + +import ( + "errors" + "reflect" + "sort" + "strings" + + "github.com/mulgadc/spinifex/spinifex/arn" + "github.com/mulgadc/spinifex/spinifex/awsec2query" + "github.com/mulgadc/spinifex/spinifex/awserrors" +) + +// The resource a policy check evaluates against when the request names nothing +// in particular: the describes, and the actions whose identifier cannot be +// resolved without a lookup the gate cannot do. +const anyResource = "*" + +// Where an action's identifier lives in its parsed SDK input and what it +// resolves to. Paths are tried in order, matching handler precedence. No paths +// means kind is a resource the action creates, whose id does not exist yet. +type resourceScope struct { + paths []string + kind arn.EC2ResourceType + byPrefix bool + optional bool +} + +// Scopes shared across actions. Names ending in New resolve to /*, which +// is what AWS evaluates for a resource the call is about to create. +// +// Where an action accepts a name as well as an id, only the id is read: +// GroupName is EC2-Classic, PublicIp is not an allocation id, and a launch +// template ARN carries the template id. A name-only request leaves the resource +// unresolved rather than building an ARN that names nothing. +var ( + instanceListScope = &resourceScope{paths: []string{"InstanceIds"}, kind: arn.EC2Instance} + instanceScope = &resourceScope{paths: []string{"InstanceId"}, kind: arn.EC2Instance} + instanceOptScope = &resourceScope{paths: []string{"InstanceId"}, kind: arn.EC2Instance, optional: true} + instanceNewScope = &resourceScope{kind: arn.EC2Instance} + + volumeScope = &resourceScope{paths: []string{"VolumeId"}, kind: arn.EC2Volume} + volumeNewScope = &resourceScope{kind: arn.EC2Volume} + + imageScope = &resourceScope{paths: []string{"ImageId"}, kind: arn.EC2Image} + imageNewScope = &resourceScope{kind: arn.EC2Image} + + snapshotScope = &resourceScope{paths: []string{"SnapshotId"}, kind: arn.EC2Snapshot} + snapshotOptScope = &resourceScope{paths: []string{"SnapshotId"}, kind: arn.EC2Snapshot, optional: true} + snapshotNewScope = &resourceScope{kind: arn.EC2Snapshot} + + vpcScope = &resourceScope{paths: []string{"VpcId"}, kind: arn.EC2VPC} + vpcOptScope = &resourceScope{paths: []string{"VpcId"}, kind: arn.EC2VPC, optional: true} + vpcNewScope = &resourceScope{kind: arn.EC2VPC} + + subnetScope = &resourceScope{paths: []string{"SubnetId"}, kind: arn.EC2Subnet} + subnetOptScope = &resourceScope{paths: []string{"SubnetId"}, kind: arn.EC2Subnet, optional: true} + runSubnetOptScope = &resourceScope{paths: []string{"SubnetId", "NetworkInterfaces.SubnetId"}, kind: arn.EC2Subnet, optional: true} + subnetNewScope = &resourceScope{kind: arn.EC2Subnet} + + securityGroupScope = &resourceScope{paths: []string{"GroupId"}, kind: arn.EC2SecurityGroup} + securityGroupsOptList = &resourceScope{paths: []string{"SecurityGroupIds", "Groups"}, kind: arn.EC2SecurityGroup, optional: true} + runSecurityGroups = &resourceScope{paths: []string{"SecurityGroupIds", "NetworkInterfaces.Groups"}, kind: arn.EC2SecurityGroup, optional: true} + securityGroupNewScope = &resourceScope{kind: arn.EC2SecurityGroup} + + routeTableScope = &resourceScope{paths: []string{"RouteTableId"}, kind: arn.EC2RouteTable} + routeTableNewScope = &resourceScope{kind: arn.EC2RouteTable} + + igwScope = &resourceScope{paths: []string{"InternetGatewayId"}, kind: arn.EC2InternetGateway} + igwNewScope = &resourceScope{kind: arn.EC2InternetGateway} + + eigwScope = &resourceScope{paths: []string{"EgressOnlyInternetGatewayId"}, kind: arn.EC2EgressOnlyInternetGateway} + eigwNewScope = &resourceScope{kind: arn.EC2EgressOnlyInternetGateway} + + eniScope = &resourceScope{paths: []string{"NetworkInterfaceId"}, kind: arn.EC2NetworkInterface} + eniOptScope = &resourceScope{paths: []string{"NetworkInterfaceId"}, kind: arn.EC2NetworkInterface, optional: true} + eniNewScope = &resourceScope{kind: arn.EC2NetworkInterface} + + addressScope = &resourceScope{paths: []string{"AllocationId"}, kind: arn.EC2ElasticIP} + addressOptScope = &resourceScope{paths: []string{"AllocationId"}, kind: arn.EC2ElasticIP, optional: true} + addressNewScope = &resourceScope{kind: arn.EC2ElasticIP} + + natGatewayScope = &resourceScope{paths: []string{"NatGatewayId"}, kind: arn.EC2NATGateway} + natGatewayOptScope = &resourceScope{paths: []string{"NatGatewayId"}, kind: arn.EC2NATGateway, optional: true} + natGatewayNewScope = &resourceScope{kind: arn.EC2NATGateway} + + keyPairScope = &resourceScope{paths: []string{"KeyPairId", "KeyName"}, kind: arn.EC2KeyPair} + keyPairOptScope = &resourceScope{paths: []string{"KeyName"}, kind: arn.EC2KeyPair, optional: true} + + placementGroupScope = &resourceScope{paths: []string{"GroupName"}, kind: arn.EC2PlacementGroup} + + launchTemplateScope = &resourceScope{paths: []string{"LaunchTemplateId"}, kind: arn.EC2LaunchTemplate} + launchTemplateNewScope = &resourceScope{kind: arn.EC2LaunchTemplate} + + capacityReservationScope = &resourceScope{paths: []string{"CapacityReservationId"}, kind: arn.EC2CapacityReservation} + capacityReservationNewScope = &resourceScope{kind: arn.EC2CapacityReservation} + + spotRequestListScope = &resourceScope{paths: []string{"SpotInstanceRequestIds"}, kind: arn.EC2SpotInstancesRequest} + spotRequestNewScope = &resourceScope{kind: arn.EC2SpotInstancesRequest} + taggedResourcesScope = &resourceScope{paths: []string{"Resources"}, byPrefix: true} + gatewayOptScope = &resourceScope{paths: []string{"GatewayId"}, byPrefix: true, optional: true} + + spotImageScope = &resourceScope{paths: []string{"LaunchSpecification.ImageId"}, kind: arn.EC2Image} + spotSubnetScope = &resourceScope{ + paths: []string{"LaunchSpecification.SubnetId", "LaunchSpecification.NetworkInterfaces.SubnetId"}, + kind: arn.EC2Subnet, + optional: true, + } + spotSecurityGroups = &resourceScope{ + paths: []string{"LaunchSpecification.SecurityGroupIds", "LaunchSpecification.NetworkInterfaces.Groups"}, + kind: arn.EC2SecurityGroup, + optional: true, + } + spotKeyPairScope = &resourceScope{paths: []string{"LaunchSpecification.KeyName"}, kind: arn.EC2KeyPair, optional: true} +) + +// unscoped is the explicit "this action authorizes account-wide" entry. It is a +// value in its own right, not a missing row: the completeness test cannot tell +// a sparse table from an incomplete one, so every action carries an entry. +var unscoped = []*resourceScope{{}} + +// ec2Scopes covers every action in the gateway's EC2 dispatch table. An action +// missing from here is a bug, not an unscoped action — see ResourceARNs. +var ec2Scopes = map[string][]*resourceScope{ + // Instances. + "RunInstances": {instanceNewScope, volumeNewScope, imageScope, runSubnetOptScope, runSecurityGroups, keyPairOptScope}, + "StartInstances": {instanceListScope}, + "StopInstances": {instanceListScope}, + "RebootInstances": {instanceListScope}, + "TerminateInstances": {instanceListScope}, + "MonitorInstances": {instanceListScope}, + "UnmonitorInstances": {instanceListScope}, + "ModifyInstanceAttribute": {instanceScope}, + "ModifyInstanceMetadataOptions": {instanceScope}, + "GetConsoleOutput": {instanceScope}, + "GetPasswordData": {instanceScope}, + "AssociateIamInstanceProfile": {instanceScope}, + + // The association id needs a NATS lookup to reach the instance behind it, so + // a Deny scoped to that instance stays inert. + "DisassociateIamInstanceProfile": unscoped, + "ReplaceIamInstanceProfileAssociation": unscoped, + + // Volumes and snapshots. + "CreateVolume": {volumeNewScope, snapshotOptScope}, + "DeleteVolume": {volumeScope}, + "ModifyVolume": {volumeScope}, + "AttachVolume": {volumeScope, instanceScope}, + "DetachVolume": {volumeScope, instanceOptScope}, + "CreateSnapshot": {snapshotNewScope, volumeScope}, + "DeleteSnapshot": {snapshotScope}, + // The source snapshot lives in SourceRegion, so an ARN built from gw.Region + // would name a resource in the wrong region. + "CopySnapshot": {snapshotNewScope}, + + // Images. + "CreateImage": {imageNewScope, instanceScope}, + "RegisterImage": {imageNewScope}, + "DeregisterImage": {imageScope}, + "ModifyImageAttribute": {imageScope}, + "ResetImageAttribute": {imageScope}, + // Source image is in SourceRegion, as with CopySnapshot. + "CopyImage": {imageNewScope}, + + // Key pairs. + "CreateKeyPair": {keyPairScope}, + "ImportKeyPair": {keyPairScope}, + "DeleteKeyPair": {keyPairScope}, + + // VPCs and subnets. + "CreateVpc": {vpcNewScope}, + "DeleteVpc": {vpcScope}, + "ModifyVpcAttribute": {vpcScope}, + "CreateSubnet": {subnetNewScope, vpcScope}, + "DeleteSubnet": {subnetScope}, + "ModifySubnetAttribute": {subnetScope}, + + // Route tables. + "CreateRouteTable": {routeTableNewScope, vpcScope}, + "DeleteRouteTable": {routeTableScope}, + "CreateRoute": {routeTableScope, gatewayOptScope, natGatewayOptScope}, + "ReplaceRoute": {routeTableScope, gatewayOptScope}, + "DeleteRoute": {routeTableScope}, + "AssociateRouteTable": {routeTableScope, subnetOptScope, gatewayOptScope}, + "ReplaceRouteTableAssociation": {routeTableScope}, + // Carries only an association id, which needs a lookup to reach the table. + "DisassociateRouteTable": unscoped, + + // Gateways. + "CreateInternetGateway": {igwNewScope}, + "DeleteInternetGateway": {igwScope}, + "AttachInternetGateway": {igwScope, vpcScope}, + "DetachInternetGateway": {igwScope, vpcScope}, + "CreateEgressOnlyInternetGateway": {eigwNewScope, vpcScope}, + "DeleteEgressOnlyInternetGateway": {eigwScope}, + "CreateNatGateway": {natGatewayNewScope, subnetScope, addressOptScope}, + "DeleteNatGateway": {natGatewayScope}, + + // Network interfaces. + "CreateNetworkInterface": {eniNewScope, subnetScope, securityGroupsOptList}, + "DeleteNetworkInterface": {eniScope}, + "ModifyNetworkInterfaceAttribute": {eniScope}, + "AttachNetworkInterface": {eniScope, instanceScope}, + // Carries only an attachment id, which needs a lookup to reach the interface. + "DetachNetworkInterface": unscoped, + + // Security groups. + "CreateSecurityGroup": {securityGroupNewScope, vpcOptScope}, + "DeleteSecurityGroup": {securityGroupScope}, + "AuthorizeSecurityGroupIngress": {securityGroupScope}, + "AuthorizeSecurityGroupEgress": {securityGroupScope}, + "RevokeSecurityGroupIngress": {securityGroupScope}, + "RevokeSecurityGroupEgress": {securityGroupScope}, + + // Addresses. + "AllocateAddress": {addressNewScope}, + "ReleaseAddress": {addressScope}, + "AssociateAddress": {addressScope, instanceOptScope, eniOptScope}, + // Carries an eipassoc- id, which needs a lookup to reach the address. + "DisassociateAddress": unscoped, + + // Placement groups. + "CreatePlacementGroup": {placementGroupScope}, + "DeletePlacementGroup": {placementGroupScope}, + + // Launch templates. + "CreateLaunchTemplate": {launchTemplateNewScope}, + "CreateLaunchTemplateVersion": {launchTemplateScope}, + "ModifyLaunchTemplate": {launchTemplateScope}, + "DeleteLaunchTemplate": {launchTemplateScope}, + "DeleteLaunchTemplateVersions": {launchTemplateScope}, + + // Capacity reservations and spot. + "CreateCapacityReservation": {capacityReservationNewScope}, + "CancelCapacityReservation": {capacityReservationScope}, + "RequestSpotInstances": {spotRequestNewScope, instanceNewScope, volumeNewScope, spotImageScope, spotSubnetScope, spotSecurityGroups, spotKeyPairScope}, + "CancelSpotInstanceRequests": {spotRequestListScope}, + + // Tags. The type comes from each id's prefix; an unrecognised prefix has no + // correct ARN, so it contributes "*" rather than a plausible-looking one. + "CreateTags": {taggedResourcesScope}, + "DeleteTags": {taggedResourcesScope}, + + // Account-level settings: no resource to name. + "EnableEbsEncryptionByDefault": unscoped, + "DisableEbsEncryptionByDefault": unscoped, + "GetEbsEncryptionByDefault": unscoped, + "EnableSerialConsoleAccess": unscoped, + "DisableSerialConsoleAccess": unscoped, + "GetSerialConsoleAccessStatus": unscoped, + + // Describes. "*" is fidelity, not a stub: EC2 describe actions do not + // support resource-level permissions in AWS either. + "DescribeAccountAttributes": unscoped, + "DescribeAddresses": unscoped, + "DescribeAddressesAttribute": unscoped, + "DescribeAvailabilityZones": unscoped, + "DescribeCapacityReservations": unscoped, + "DescribeEgressOnlyInternetGateways": unscoped, + "DescribeIamInstanceProfileAssociations": unscoped, + "DescribeImageAttribute": unscoped, + "DescribeImages": unscoped, + "DescribeInstanceAttribute": unscoped, + "DescribeInstanceCreditSpecifications": unscoped, + "DescribeInstanceStatus": unscoped, + "DescribeInstanceTypes": unscoped, + "DescribeInstances": unscoped, + "DescribeInternetGateways": unscoped, + "DescribeKeyPairs": unscoped, + "DescribeLaunchTemplateVersions": unscoped, + "DescribeLaunchTemplates": unscoped, + "DescribeNatGateways": unscoped, + "DescribeNetworkInterfaces": unscoped, + "DescribePlacementGroups": unscoped, + "DescribeRegions": unscoped, + "DescribeRouteTables": unscoped, + "DescribeSecurityGroupRules": unscoped, + "DescribeSecurityGroups": unscoped, + "DescribeSnapshots": unscoped, + "DescribeSpotInstanceRequests": unscoped, + "DescribeSubnets": unscoped, + "DescribeTags": unscoped, + "DescribeVolumeStatus": unscoped, + "DescribeVolumes": unscoped, + "DescribeVolumesModifications": unscoped, + "DescribeVpcAttribute": unscoped, + "DescribeVpcs": unscoped, +} + +// HasScope reports whether the action has a scope table entry. ec2Actions lives +// in package gateway, so its completeness test needs this. +func HasScope(action string) bool { + _, ok := ec2Scopes[action] + return ok +} + +// ScopedActions returns every action the scope table covers, so a scope left +// behind by a deleted or renamed action fails the completeness test too. +func ScopedActions() []string { + actions := make([]string, 0, len(ec2Scopes)) + for action := range ec2Scopes { + actions = append(actions, action) + } + sort.Strings(actions) + return actions +} + +// ResourceARNs builds every resource the action's policy check evaluates from +// the same parsed SDK input the handler receives. A missing action is a bug. +func ResourceARNs(action, region, accountID string, input any) ([]string, error) { + scopes, ok := ec2Scopes[action] + if !ok { + return nil, errors.New(awserrors.ErrorInvalidAction) + } + + resources := make([]string, 0, len(scopes)) + seen := make(map[string]struct{}) + for _, scope := range scopes { + resolved, err := scope.resolve(region, accountID, input) + if err != nil { + return nil, err + } + for _, resource := range resolved { + if _, ok := seen[resource]; ok { + continue + } + seen[resource] = struct{}{} + resources = append(resources, resource) + if len(resources) > awsec2query.MaxSliceLen { + return nil, errors.New(awserrors.ErrorMalformedQueryString) + } + } + } + if len(resources) == 0 { + return []string{anyResource}, nil + } + return resources, nil +} + +// resolve returns the ARNs one scope contributes, which may be none. +func (s *resourceScope) resolve(region, accountID string, input any) ([]string, error) { + if len(s.paths) == 0 { + if s.kind == "" { + return []string{anyResource}, nil + } + return []string{arn.FormatEC2(s.kind, region, accountID, anyResource)}, nil + } + + ids := s.identifiers(input) + if len(ids) == 0 { + if s.optional { + return nil, nil + } + return []string{anyResource}, nil + } + if len(ids) > awsec2query.MaxSliceLen { + return nil, errors.New(awserrors.ErrorMalformedQueryString) + } + + resources := make([]string, 0, len(ids)) + for _, id := range ids { + kind := s.kind + if s.byPrefix { + resolved, ok := arn.EC2TypeForID(id) + if !ok { + resources = append(resources, anyResource) + continue + } + kind = resolved + } + resources = append(resources, arn.FormatEC2(kind, region, accountID, id)) + } + return resources, nil +} + +// identifiers reads the first populated field path, matching handler +// precedence. Values are deduplicated before policy evaluation. +func (s *resourceScope) identifiers(input any) []string { + for _, path := range s.paths { + values := stringValuesAt(reflect.ValueOf(input), strings.Split(path, ".")) + seen := make(map[string]struct{}, len(values)) + ids := make([]string, 0, len(values)) + for _, value := range values { + if value == "" { + continue + } + if _, ok := seen[value]; ok { + continue + } + seen[value] = struct{}{} + ids = append(ids, value) + } + if len(ids) > 0 { + return ids + } + } + return nil +} + +func stringValuesAt(value reflect.Value, path []string) []string { + for value.IsValid() && (value.Kind() == reflect.Pointer || value.Kind() == reflect.Interface) { + if value.IsNil() { + return nil + } + value = value.Elem() + } + if !value.IsValid() { + return nil + } + if value.Kind() == reflect.Slice || value.Kind() == reflect.Array { + var values []string + for i := 0; i < value.Len(); i++ { + values = append(values, stringValuesAt(value.Index(i), path)...) + } + return values + } + if len(path) == 0 { + if value.Kind() == reflect.String { + return []string{value.String()} + } + return nil + } + if value.Kind() != reflect.Struct { + return nil + } + return stringValuesAt(value.FieldByName(path[0]), path[1:]) +} diff --git a/spinifex/gateway/ec2/authz_test.go b/spinifex/gateway/ec2/authz_test.go new file mode 100644 index 000000000..6b82d24f0 --- /dev/null +++ b/spinifex/gateway/ec2/authz_test.go @@ -0,0 +1,440 @@ +//test:in-package — the fidelity test reads ec2Scopes and each scope's +// unexported params to prove they name a parameter the handler parses. + +package gateway_ec2 + +import ( + "reflect" + "strings" + "testing" + + "github.com/aws/aws-sdk-go/service/ec2" + "github.com/mulgadc/spinifex/spinifex/awsec2query" + "github.com/mulgadc/spinifex/spinifex/awserrors" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +const ( + testRegion = "ap-southeast-2" + testAccountID = "123456789012" +) + +func ec2arn(kind, id string) string { + return "arn:aws:ec2:" + testRegion + ":" + testAccountID + ":" + kind + "/" + id +} + +func resolve(t *testing.T, action string, q map[string]string) []string { + t.Helper() + var input any + if prototype, ok := ec2Inputs[action]; ok { + input = reflect.New(reflect.TypeOf(prototype).Elem()).Interface() + require.NoError(t, awsec2query.QueryParamsToStruct(q, input)) + } + got, err := ResourceARNs(action, testRegion, testAccountID, input) + require.NoError(t, err) + return got +} + +// TestResourceARNs pins the ARN each scoped action hands the evaluator. The +// order is the scope order, which the denial log reproduces. +func TestResourceARNs(t *testing.T) { + tests := []struct { + action string + q map[string]string + want []string + }{ + // Instances. + {"RunInstances", map[string]string{ + "ImageId": "ami-1", "SubnetId": "subnet-1", "SecurityGroupId.1": "sg-1", "KeyName": "k1", + }, []string{ + ec2arn("instance", "*"), ec2arn("volume", "*"), ec2arn("image", "ami-1"), + ec2arn("subnet", "subnet-1"), ec2arn("security-group", "sg-1"), ec2arn("key-pair", "k1"), + }}, + {"RunInstances", map[string]string{"ImageId": "ami-1"}, + []string{ec2arn("instance", "*"), ec2arn("volume", "*"), ec2arn("image", "ami-1")}}, + {"RunInstances", map[string]string{ + "ImageId": "ami-1", "NetworkInterface.1.SubnetId": "subnet-1", + "NetworkInterface.1.SecurityGroupId.1": "sg-1", + }, []string{ + ec2arn("instance", "*"), ec2arn("volume", "*"), ec2arn("image", "ami-1"), + ec2arn("subnet", "subnet-1"), ec2arn("security-group", "sg-1"), + }}, + {"StartInstances", map[string]string{"InstanceId.1": "i-1"}, []string{ec2arn("instance", "i-1")}}, + {"StopInstances", map[string]string{"InstanceId.1": "i-1"}, []string{ec2arn("instance", "i-1")}}, + {"RebootInstances", map[string]string{"InstanceId.1": "i-1"}, []string{ec2arn("instance", "i-1")}}, + {"MonitorInstances", map[string]string{"InstanceId.1": "i-1"}, []string{ec2arn("instance", "i-1")}}, + {"UnmonitorInstances", map[string]string{"InstanceId.1": "i-1"}, []string{ec2arn("instance", "i-1")}}, + {"TerminateInstances", map[string]string{"InstanceId.1": "i-1", "InstanceId.2": "i-2"}, + []string{ec2arn("instance", "i-1"), ec2arn("instance", "i-2")}}, + {"ModifyInstanceAttribute", map[string]string{"InstanceId": "i-1"}, []string{ec2arn("instance", "i-1")}}, + {"ModifyInstanceAttribute", map[string]string{"instanceId": "i-2"}, []string{ec2arn("instance", "i-2")}}, + {"ModifyInstanceMetadataOptions", map[string]string{"InstanceId": "i-1"}, []string{ec2arn("instance", "i-1")}}, + {"GetConsoleOutput", map[string]string{"InstanceId": "i-1"}, []string{ec2arn("instance", "i-1")}}, + {"GetPasswordData", map[string]string{"InstanceId": "i-1"}, []string{ec2arn("instance", "i-1")}}, + {"AssociateIamInstanceProfile", map[string]string{"InstanceId": "i-1"}, []string{ec2arn("instance", "i-1")}}, + {"DisassociateIamInstanceProfile", map[string]string{"AssociationId": "iip-assoc-1"}, []string{"*"}}, + {"ReplaceIamInstanceProfileAssociation", map[string]string{"AssociationId": "iip-assoc-1"}, []string{"*"}}, + + // Volumes and snapshots. + {"CreateVolume", map[string]string{}, []string{ec2arn("volume", "*")}}, + {"CreateVolume", map[string]string{"SnapshotId": "snap-1"}, + []string{ec2arn("volume", "*"), ec2arn("snapshot", "snap-1")}}, + {"DeleteVolume", map[string]string{"VolumeId": "vol-1"}, []string{ec2arn("volume", "vol-1")}}, + {"ModifyVolume", map[string]string{"VolumeId": "vol-1"}, []string{ec2arn("volume", "vol-1")}}, + {"AttachVolume", map[string]string{"VolumeId": "vol-1", "InstanceId": "i-1"}, + []string{ec2arn("volume", "vol-1"), ec2arn("instance", "i-1")}}, + {"DetachVolume", map[string]string{"VolumeId": "vol-1"}, []string{ec2arn("volume", "vol-1")}}, + {"DetachVolume", map[string]string{"VolumeId": "vol-1", "InstanceId": "i-1"}, + []string{ec2arn("volume", "vol-1"), ec2arn("instance", "i-1")}}, + {"CreateSnapshot", map[string]string{"VolumeId": "vol-1"}, + []string{ec2arn("snapshot", "*"), ec2arn("volume", "vol-1")}}, + {"DeleteSnapshot", map[string]string{"SnapshotId": "snap-1"}, []string{ec2arn("snapshot", "snap-1")}}, + {"CopySnapshot", map[string]string{"SourceSnapshotId": "snap-1", "SourceRegion": "us-east-1"}, + []string{ec2arn("snapshot", "*")}}, + + // Images. + {"CreateImage", map[string]string{"InstanceId": "i-1"}, + []string{ec2arn("image", "*"), ec2arn("instance", "i-1")}}, + {"RegisterImage", map[string]string{"Name": "img"}, []string{ec2arn("image", "*")}}, + {"CopyImage", map[string]string{"SourceImageId": "ami-1", "SourceRegion": "us-east-1"}, + []string{ec2arn("image", "*")}}, + {"DeregisterImage", map[string]string{"ImageId": "ami-1"}, []string{ec2arn("image", "ami-1")}}, + {"ModifyImageAttribute", map[string]string{"ImageId": "ami-1"}, []string{ec2arn("image", "ami-1")}}, + {"ResetImageAttribute", map[string]string{"ImageId": "ami-1"}, []string{ec2arn("image", "ami-1")}}, + + // Key pairs. Either parameter names the same resource, so neither + // contributes a spurious "*" when the other is the one supplied. + {"CreateKeyPair", map[string]string{"KeyName": "k1"}, []string{ec2arn("key-pair", "k1")}}, + {"ImportKeyPair", map[string]string{"KeyName": "k1"}, []string{ec2arn("key-pair", "k1")}}, + {"DeleteKeyPair", map[string]string{"KeyName": "k1"}, []string{ec2arn("key-pair", "k1")}}, + {"DeleteKeyPair", map[string]string{"KeyPairId": "key-1"}, []string{ec2arn("key-pair", "key-1")}}, + {"DeleteKeyPair", map[string]string{"KeyName": "k1", "KeyPairId": "key-1"}, []string{ec2arn("key-pair", "key-1")}}, + + // VPCs and subnets. + {"CreateVpc", map[string]string{"CidrBlock": "10.0.0.0/16"}, []string{ec2arn("vpc", "*")}}, + {"DeleteVpc", map[string]string{"VpcId": "vpc-1"}, []string{ec2arn("vpc", "vpc-1")}}, + {"ModifyVpcAttribute", map[string]string{"VpcId": "vpc-1"}, []string{ec2arn("vpc", "vpc-1")}}, + {"CreateSubnet", map[string]string{"VpcId": "vpc-1"}, + []string{ec2arn("subnet", "*"), ec2arn("vpc", "vpc-1")}}, + {"DeleteSubnet", map[string]string{"SubnetId": "subnet-1"}, []string{ec2arn("subnet", "subnet-1")}}, + {"ModifySubnetAttribute", map[string]string{"SubnetId": "subnet-1"}, []string{ec2arn("subnet", "subnet-1")}}, + + // Route tables. + {"CreateRouteTable", map[string]string{"VpcId": "vpc-1"}, + []string{ec2arn("route-table", "*"), ec2arn("vpc", "vpc-1")}}, + {"DeleteRouteTable", map[string]string{"RouteTableId": "rtb-1"}, []string{ec2arn("route-table", "rtb-1")}}, + {"DeleteRoute", map[string]string{"RouteTableId": "rtb-1"}, []string{ec2arn("route-table", "rtb-1")}}, + {"CreateRoute", map[string]string{"RouteTableId": "rtb-1", "GatewayId": "igw-1"}, + []string{ec2arn("route-table", "rtb-1"), ec2arn("internet-gateway", "igw-1")}}, + {"CreateRoute", map[string]string{"RouteTableId": "rtb-1", "NatGatewayId": "nat-1"}, + []string{ec2arn("route-table", "rtb-1"), ec2arn("natgateway", "nat-1")}}, + {"ReplaceRoute", map[string]string{"RouteTableId": "rtb-1", "GatewayId": "igw-1"}, + []string{ec2arn("route-table", "rtb-1"), ec2arn("internet-gateway", "igw-1")}}, + {"AssociateRouteTable", map[string]string{"RouteTableId": "rtb-1", "SubnetId": "subnet-1"}, + []string{ec2arn("route-table", "rtb-1"), ec2arn("subnet", "subnet-1")}}, + {"ReplaceRouteTableAssociation", map[string]string{"RouteTableId": "rtb-1", "AssociationId": "rtbassoc-1"}, + []string{ec2arn("route-table", "rtb-1")}}, + {"DisassociateRouteTable", map[string]string{"AssociationId": "rtbassoc-1"}, []string{"*"}}, + + // Gateways. + {"CreateInternetGateway", map[string]string{}, []string{ec2arn("internet-gateway", "*")}}, + {"DeleteInternetGateway", map[string]string{"InternetGatewayId": "igw-1"}, + []string{ec2arn("internet-gateway", "igw-1")}}, + {"AttachInternetGateway", map[string]string{"InternetGatewayId": "igw-1", "VpcId": "vpc-1"}, + []string{ec2arn("internet-gateway", "igw-1"), ec2arn("vpc", "vpc-1")}}, + {"DetachInternetGateway", map[string]string{"InternetGatewayId": "igw-1", "VpcId": "vpc-1"}, + []string{ec2arn("internet-gateway", "igw-1"), ec2arn("vpc", "vpc-1")}}, + {"CreateEgressOnlyInternetGateway", map[string]string{"VpcId": "vpc-1"}, + []string{ec2arn("egress-only-internet-gateway", "*"), ec2arn("vpc", "vpc-1")}}, + {"DeleteEgressOnlyInternetGateway", map[string]string{"EgressOnlyInternetGatewayId": "eigw-1"}, + []string{ec2arn("egress-only-internet-gateway", "eigw-1")}}, + {"CreateNatGateway", map[string]string{"SubnetId": "subnet-1", "AllocationId": "eipalloc-1"}, + []string{ec2arn("natgateway", "*"), ec2arn("subnet", "subnet-1"), ec2arn("elastic-ip", "eipalloc-1")}}, + {"DeleteNatGateway", map[string]string{"NatGatewayId": "nat-1"}, []string{ec2arn("natgateway", "nat-1")}}, + + // Network interfaces. + {"CreateNetworkInterface", map[string]string{"SubnetId": "subnet-1", "SecurityGroupId.1": "sg-1"}, + []string{ec2arn("network-interface", "*"), ec2arn("subnet", "subnet-1"), ec2arn("security-group", "sg-1")}}, + {"CreateNetworkInterface", map[string]string{ + "SubnetId": "subnet-1", "Groups.1": "sg-handler", "SecurityGroupId.1": "sg-other", + }, []string{ + ec2arn("network-interface", "*"), ec2arn("subnet", "subnet-1"), ec2arn("security-group", "sg-handler"), + }}, + {"DeleteNetworkInterface", map[string]string{"NetworkInterfaceId": "eni-1"}, + []string{ec2arn("network-interface", "eni-1")}}, + {"ModifyNetworkInterfaceAttribute", map[string]string{"NetworkInterfaceId": "eni-1"}, + []string{ec2arn("network-interface", "eni-1")}}, + {"AttachNetworkInterface", map[string]string{"NetworkInterfaceId": "eni-1", "InstanceId": "i-1"}, + []string{ec2arn("network-interface", "eni-1"), ec2arn("instance", "i-1")}}, + {"DetachNetworkInterface", map[string]string{"AttachmentId": "eni-attach-1"}, []string{"*"}}, + + // Security groups. + {"CreateSecurityGroup", map[string]string{"VpcId": "vpc-1"}, + []string{ec2arn("security-group", "*"), ec2arn("vpc", "vpc-1")}}, + {"CreateSecurityGroup", map[string]string{"GroupName": "web"}, []string{ec2arn("security-group", "*")}}, + {"DeleteSecurityGroup", map[string]string{"GroupId": "sg-1"}, []string{ec2arn("security-group", "sg-1")}}, + {"AuthorizeSecurityGroupIngress", map[string]string{"GroupId": "sg-1"}, []string{ec2arn("security-group", "sg-1")}}, + {"AuthorizeSecurityGroupEgress", map[string]string{"GroupId": "sg-1"}, []string{ec2arn("security-group", "sg-1")}}, + {"RevokeSecurityGroupIngress", map[string]string{"GroupId": "sg-1"}, []string{ec2arn("security-group", "sg-1")}}, + {"RevokeSecurityGroupEgress", map[string]string{"GroupId": "sg-1"}, []string{ec2arn("security-group", "sg-1")}}, + + // Addresses. + {"AllocateAddress", map[string]string{"Domain": "vpc"}, []string{ec2arn("elastic-ip", "*")}}, + {"ReleaseAddress", map[string]string{"AllocationId": "eipalloc-1"}, []string{ec2arn("elastic-ip", "eipalloc-1")}}, + {"AssociateAddress", map[string]string{"AllocationId": "eipalloc-1", "InstanceId": "i-1"}, + []string{ec2arn("elastic-ip", "eipalloc-1"), ec2arn("instance", "i-1")}}, + {"AssociateAddress", map[string]string{"AllocationId": "eipalloc-1", "NetworkInterfaceId": "eni-1"}, + []string{ec2arn("elastic-ip", "eipalloc-1"), ec2arn("network-interface", "eni-1")}}, + {"DisassociateAddress", map[string]string{"AssociationId": "eipassoc-1"}, []string{"*"}}, + + // Placement groups. + {"CreatePlacementGroup", map[string]string{"GroupName": "pg1"}, []string{ec2arn("placement-group", "pg1")}}, + {"DeletePlacementGroup", map[string]string{"GroupName": "pg1"}, []string{ec2arn("placement-group", "pg1")}}, + + // Launch templates. + {"CreateLaunchTemplate", map[string]string{"LaunchTemplateName": "lt"}, + []string{ec2arn("launch-template", "*")}}, + {"CreateLaunchTemplateVersion", map[string]string{"LaunchTemplateId": "lt-1"}, + []string{ec2arn("launch-template", "lt-1")}}, + {"ModifyLaunchTemplate", map[string]string{"LaunchTemplateId": "lt-1"}, + []string{ec2arn("launch-template", "lt-1")}}, + {"DeleteLaunchTemplate", map[string]string{"LaunchTemplateId": "lt-1"}, + []string{ec2arn("launch-template", "lt-1")}}, + {"DeleteLaunchTemplateVersions", map[string]string{"LaunchTemplateId": "lt-1"}, + []string{ec2arn("launch-template", "lt-1")}}, + + // Capacity reservations and spot. + {"CreateCapacityReservation", map[string]string{"InstanceType": "t3.micro"}, + []string{ec2arn("capacity-reservation", "*")}}, + {"CancelCapacityReservation", map[string]string{"CapacityReservationId": "cr-1"}, + []string{ec2arn("capacity-reservation", "cr-1")}}, + {"RequestSpotInstances", map[string]string{ + "LaunchSpecification.ImageId": "ami-1", "LaunchSpecification.SubnetId": "subnet-1", + "LaunchSpecification.SecurityGroupId.1": "sg-1", "LaunchSpecification.KeyName": "k1", + }, []string{ + ec2arn("spot-instances-request", "*"), ec2arn("instance", "*"), ec2arn("volume", "*"), + ec2arn("image", "ami-1"), ec2arn("subnet", "subnet-1"), + ec2arn("security-group", "sg-1"), ec2arn("key-pair", "k1"), + }}, + {"CancelSpotInstanceRequests", map[string]string{"SpotInstanceRequestId.1": "sir-1"}, + []string{ec2arn("spot-instances-request", "sir-1")}}, + + // Tags: the type comes from each id's prefix, one member per id. + {"CreateTags", map[string]string{"ResourceId.1": "i-1", "ResourceId.2": "vol-1"}, + []string{ec2arn("instance", "i-1"), ec2arn("volume", "vol-1")}}, + {"DeleteTags", map[string]string{"ResourceId.1": "sg-1"}, []string{ec2arn("security-group", "sg-1")}}, + // An untypable id has no correct ARN, so the fence stays inert rather + // than fencing a plausible-looking wrong object. + {"CreateTags", map[string]string{"ResourceId.1": "i-1", "ResourceId.2": "nope-1"}, + []string{ec2arn("instance", "i-1"), "*"}}, + + // Account-level settings name no resource. + {"EnableEbsEncryptionByDefault", map[string]string{}, []string{"*"}}, + {"DisableEbsEncryptionByDefault", map[string]string{}, []string{"*"}}, + {"GetEbsEncryptionByDefault", map[string]string{}, []string{"*"}}, + {"EnableSerialConsoleAccess", map[string]string{}, []string{"*"}}, + {"DisableSerialConsoleAccess", map[string]string{}, []string{"*"}}, + {"GetSerialConsoleAccessStatus", map[string]string{}, []string{"*"}}, + + // Describes evaluate account-wide, as they do in AWS. + {"DescribeInstances", map[string]string{"InstanceId.1": "i-1"}, []string{"*"}}, + {"DescribeVolumes", map[string]string{"VolumeId.1": "vol-1"}, []string{"*"}}, + } + + for _, tc := range tests { + t.Run(tc.action, func(t *testing.T) { + assert.Equal(t, tc.want, resolve(t, tc.action, tc.q)) + }) + } +} + +// TestResourceARNsMissingIdentifier pins the property most easily lost: an +// absent identifier stays the handler's validation fault rather than becoming an +// authorization failure here. +func TestResourceARNsMissingIdentifier(t *testing.T) { + assert.Equal(t, []string{"*"}, resolve(t, "TerminateInstances", map[string]string{})) + assert.Equal(t, []string{"*"}, resolve(t, "DeleteVolume", map[string]string{})) + // GroupName names a security group only in EC2-Classic, which has no ARN. + assert.Equal(t, []string{"*"}, resolve(t, "DeleteSecurityGroup", map[string]string{"GroupName": "web"})) + // A name-only launch template request leaves the id unresolved. + assert.Equal(t, []string{"*"}, resolve(t, "DeleteLaunchTemplate", map[string]string{"LaunchTemplateName": "lt"})) +} + +func TestResourceARNsUnknownAction(t *testing.T) { + _, err := ResourceARNs("NotAnAction", testRegion, testAccountID, nil) + require.Error(t, err) + assert.Equal(t, awserrors.ErrorInvalidAction, err.Error()) +} + +// TestParsedListInputs covers the list forms and ordering produced by the +// canonical query parser used by authorization and dispatch. +func TestParsedListInputs(t *testing.T) { + t.Run("member wrapper", func(t *testing.T) { + assert.Equal(t, []string{ec2arn("instance", "i-1")}, + resolve(t, "TerminateInstances", map[string]string{"InstanceId.member.1": "i-1"})) + }) + + t.Run("field-name alias", func(t *testing.T) { + assert.Equal(t, []string{ec2arn("instance", "i-1")}, + resolve(t, "TerminateInstances", map[string]string{"InstanceIds.1": "i-1"})) + }) + + t.Run("location-name-list wrapper", func(t *testing.T) { + assert.Equal(t, []string{ec2arn("instance", "i-1")}, + resolve(t, "TerminateInstances", map[string]string{"InstanceId.InstanceId.1": "i-1"})) + }) + + t.Run("numeric order", func(t *testing.T) { + q := map[string]string{"InstanceId.1": "i-1", "InstanceId.2": "i-2", "InstanceId.3": "i-3"} + assert.Equal(t, []string{ec2arn("instance", "i-1"), ec2arn("instance", "i-2"), ec2arn("instance", "i-3")}, + resolve(t, "TerminateInstances", q)) + }) + + t.Run("gap terminates the parsed list", func(t *testing.T) { + q := map[string]string{"InstanceId.1": "i-1", "InstanceId.3": "i-3"} + assert.Equal(t, []string{ec2arn("instance", "i-1")}, resolve(t, "TerminateInstances", q)) + }) + + t.Run("nested keys are not collected", func(t *testing.T) { + q := map[string]string{"ResourceId.1": "i-1", "ResourceId.1.Value.1": "ignored"} + assert.Equal(t, []string{ec2arn("instance", "i-1")}, resolve(t, "CreateTags", q)) + }) +} + +// TestResourceARNsIdentifierIsAValue covers the two shapes from the reverted STS +// work. Both build a value, not a pattern: an id containing / is not truncated, +// and an id that is literally * neither matches a scoped Deny nor widens a grant. +func TestResourceARNsIdentifierIsAValue(t *testing.T) { + assert.Equal(t, []string{ec2arn("instance", "i-1/admin")}, + resolve(t, "TerminateInstances", map[string]string{"InstanceId.1": "i-1/admin"})) + assert.Equal(t, []string{ec2arn("instance", "*")}, + resolve(t, "TerminateInstances", map[string]string{"InstanceId.1": "*"})) +} + +// ec2Inputs is the input shape each scoped action's handler parses. It exists so +// the fidelity test below can prove the resolver reads a parameter the handler +// reads too, rather than checking the table against itself. +var ec2Inputs = map[string]any{ + "AllocateAddress": &ec2.AllocateAddressInput{}, + "AssociateAddress": &ec2.AssociateAddressInput{}, + "AssociateIamInstanceProfile": &ec2.AssociateIamInstanceProfileInput{}, + "AssociateRouteTable": &ec2.AssociateRouteTableInput{}, + "AttachInternetGateway": &ec2.AttachInternetGatewayInput{}, + "AttachNetworkInterface": &ec2.AttachNetworkInterfaceInput{}, + "AttachVolume": &ec2.AttachVolumeInput{}, + "AuthorizeSecurityGroupEgress": &ec2.AuthorizeSecurityGroupEgressInput{}, + "AuthorizeSecurityGroupIngress": &ec2.AuthorizeSecurityGroupIngressInput{}, + "CancelCapacityReservation": &ec2.CancelCapacityReservationInput{}, + "CancelSpotInstanceRequests": &ec2.CancelSpotInstanceRequestsInput{}, + "CopyImage": &ec2.CopyImageInput{}, + "CopySnapshot": &ec2.CopySnapshotInput{}, + "CreateCapacityReservation": &ec2.CreateCapacityReservationInput{}, + "CreateEgressOnlyInternetGateway": &ec2.CreateEgressOnlyInternetGatewayInput{}, + "CreateImage": &ec2.CreateImageInput{}, + "CreateInternetGateway": &ec2.CreateInternetGatewayInput{}, + "CreateKeyPair": &ec2.CreateKeyPairInput{}, + "CreateLaunchTemplate": &ec2.CreateLaunchTemplateInput{}, + "CreateLaunchTemplateVersion": &ec2.CreateLaunchTemplateVersionInput{}, + "CreateNatGateway": &ec2.CreateNatGatewayInput{}, + "CreateNetworkInterface": &ec2.CreateNetworkInterfaceInput{}, + "CreatePlacementGroup": &ec2.CreatePlacementGroupInput{}, + "CreateRoute": &ec2.CreateRouteInput{}, + "CreateRouteTable": &ec2.CreateRouteTableInput{}, + "CreateSecurityGroup": &ec2.CreateSecurityGroupInput{}, + "CreateSnapshot": &ec2.CreateSnapshotInput{}, + "CreateSubnet": &ec2.CreateSubnetInput{}, + "CreateTags": &ec2.CreateTagsInput{}, + "CreateVolume": &ec2.CreateVolumeInput{}, + "CreateVpc": &ec2.CreateVpcInput{}, + "DeleteEgressOnlyInternetGateway": &ec2.DeleteEgressOnlyInternetGatewayInput{}, + "DeleteInternetGateway": &ec2.DeleteInternetGatewayInput{}, + "DeleteKeyPair": &ec2.DeleteKeyPairInput{}, + "DeleteLaunchTemplate": &ec2.DeleteLaunchTemplateInput{}, + "DeleteLaunchTemplateVersions": &ec2.DeleteLaunchTemplateVersionsInput{}, + "DeleteNatGateway": &ec2.DeleteNatGatewayInput{}, + "DeleteNetworkInterface": &ec2.DeleteNetworkInterfaceInput{}, + "DeletePlacementGroup": &ec2.DeletePlacementGroupInput{}, + "DeleteRoute": &ec2.DeleteRouteInput{}, + "DeleteRouteTable": &ec2.DeleteRouteTableInput{}, + "DeleteSecurityGroup": &ec2.DeleteSecurityGroupInput{}, + "DeleteSnapshot": &ec2.DeleteSnapshotInput{}, + "DeleteSubnet": &ec2.DeleteSubnetInput{}, + "DeleteTags": &ec2.DeleteTagsInput{}, + "DeleteVolume": &ec2.DeleteVolumeInput{}, + "DeleteVpc": &ec2.DeleteVpcInput{}, + "DeregisterImage": &ec2.DeregisterImageInput{}, + "DetachInternetGateway": &ec2.DetachInternetGatewayInput{}, + "DetachNetworkInterface": &ec2.DetachNetworkInterfaceInput{}, + "DetachVolume": &ec2.DetachVolumeInput{}, + "DisassociateAddress": &ec2.DisassociateAddressInput{}, + "DisassociateIamInstanceProfile": &ec2.DisassociateIamInstanceProfileInput{}, + "DisassociateRouteTable": &ec2.DisassociateRouteTableInput{}, + "GetConsoleOutput": &ec2.GetConsoleOutputInput{}, + "GetPasswordData": &ec2.GetPasswordDataInput{}, + "ImportKeyPair": &ec2.ImportKeyPairInput{}, + "ModifyImageAttribute": &ec2.ModifyImageAttributeInput{}, + "ModifyInstanceAttribute": &ec2.ModifyInstanceAttributeInput{}, + "ModifyInstanceMetadataOptions": &ec2.ModifyInstanceMetadataOptionsInput{}, + "ModifyLaunchTemplate": &ec2.ModifyLaunchTemplateInput{}, + "ModifyNetworkInterfaceAttribute": &ec2.ModifyNetworkInterfaceAttributeInput{}, + "ModifySubnetAttribute": &ec2.ModifySubnetAttributeInput{}, + "ModifyVolume": &ec2.ModifyVolumeInput{}, + "ModifyVpcAttribute": &ec2.ModifyVpcAttributeInput{}, + "MonitorInstances": &ec2.MonitorInstancesInput{}, + "RebootInstances": &ec2.RebootInstancesInput{}, + "RegisterImage": &ec2.RegisterImageInput{}, + "ReleaseAddress": &ec2.ReleaseAddressInput{}, + "ReplaceIamInstanceProfileAssociation": &ec2.ReplaceIamInstanceProfileAssociationInput{}, + "ReplaceRoute": &ec2.ReplaceRouteInput{}, + "ReplaceRouteTableAssociation": &ec2.ReplaceRouteTableAssociationInput{}, + "RequestSpotInstances": &ec2.RequestSpotInstancesInput{}, + "ResetImageAttribute": &ec2.ResetImageAttributeInput{}, + "RevokeSecurityGroupEgress": &ec2.RevokeSecurityGroupEgressInput{}, + "RevokeSecurityGroupIngress": &ec2.RevokeSecurityGroupIngressInput{}, + "RunInstances": &ec2.RunInstancesInput{}, + "StartInstances": &ec2.StartInstancesInput{}, + "StopInstances": &ec2.StopInstancesInput{}, + "TerminateInstances": &ec2.TerminateInstancesInput{}, + "UnmonitorInstances": &ec2.UnmonitorInstancesInput{}, +} + +// TestScopePathsReachTheHandlerInput proves each resolver path exists in the +// typed input consumed by the action's handler. +func TestScopePathsReachTheHandlerInput(t *testing.T) { + for action, scopes := range ec2Scopes { + input, ok := ec2Inputs[action] + if !ok { + continue + } + for i, scope := range scopes { + if len(scope.paths) == 0 { + continue + } + t.Run(action, func(t *testing.T) { + var reachable []string + for _, path := range scope.paths { + if hasFieldPath(reflect.TypeOf(input), strings.Split(path, ".")) { + reachable = append(reachable, path) + } + } + assert.NotEmpty(t, reachable, + "%s scope %d paths %v do not reach its handler input", action, i, scope.paths) + }) + } + } +} + +func hasFieldPath(typ reflect.Type, path []string) bool { + for typ.Kind() == reflect.Pointer || typ.Kind() == reflect.Slice || typ.Kind() == reflect.Array { + typ = typ.Elem() + } + if len(path) == 0 { + return typ.Kind() == reflect.String + } + if typ.Kind() != reflect.Struct { + return false + } + field, ok := typ.FieldByName(path[0]) + return ok && hasFieldPath(field.Type, path[1:]) +} diff --git a/spinifex/gateway/ec2/instance/RunInstances.go b/spinifex/gateway/ec2/instance/RunInstances.go index 2e1c21277..457b18c4f 100644 --- a/spinifex/gateway/ec2/instance/RunInstances.go +++ b/spinifex/gateway/ec2/instance/RunInstances.go @@ -79,7 +79,7 @@ func ValidateRunInstancesInput(input *ec2.RunInstancesInput) (err error) { func RunInstances(ctx context.Context, input *ec2.RunInstancesInput, natsConn *nats.Conn, iamSvc handlers_iam.IAMService, accountID string, passRoleCheck PassRoleChecker, launchQuotaCheck LaunchQuotaChecker, expectedNodes int) (reservation ec2.Reservation, err error) { // Expand a referenced launch template before validation so validation, quota, // and downstream routing all see the effective (post-merge) parameters. - if err = expandLaunchTemplate(ctx, natsConn, input, accountID); err != nil { + if err = ExpandLaunchTemplate(ctx, natsConn, input, accountID); err != nil { return reservation, err } @@ -243,9 +243,9 @@ func capacityReservationTargetID(input *ec2.RunInstancesInput) string { return aws.StringValue(spec.CapacityReservationTarget.CapacityReservationId) } -// expandLaunchTemplate folds a referenced launch template into input, delegating +// ExpandLaunchTemplate folds a referenced launch template into input, delegating // to the launch-template service over NATS. A no-op when no template is referenced. -func expandLaunchTemplate(ctx context.Context, natsConn *nats.Conn, input *ec2.RunInstancesInput, accountID string) error { +func ExpandLaunchTemplate(ctx context.Context, natsConn *nats.Conn, input *ec2.RunInstancesInput, accountID string) error { if input == nil || input.LaunchTemplate == nil { return nil } diff --git a/spinifex/gateway/ec2_authz_completeness_test.go b/spinifex/gateway/ec2_authz_completeness_test.go new file mode 100644 index 000000000..9b086e739 --- /dev/null +++ b/spinifex/gateway/ec2_authz_completeness_test.go @@ -0,0 +1,27 @@ +//test:in-package — ec2Actions is unexported here, and the completeness test +// exists to compare it against the scope table. + +package gateway + +import ( + "testing" + + gateway_ec2 "github.com/mulgadc/spinifex/spinifex/gateway/ec2" + "github.com/stretchr/testify/assert" +) + +// TestEC2ScopeTableIsExhaustive is what stops the next EC2 action being added +// with a silent account-wide grant. It asserts both directions, so a scope left +// behind by a deleted or renamed action fails too. +func TestEC2ScopeTableIsExhaustive(t *testing.T) { + for action := range ec2Actions { + assert.True(t, gateway_ec2.HasScope(action), + "ec2 action %q has no resource scope entry: add one to ec2Scopes in gateway/ec2/authz.go", action) + } + + for _, action := range gateway_ec2.ScopedActions() { + _, ok := ec2Actions[action] + assert.True(t, ok, + "ec2Scopes has an entry for %q, which the dispatch table does not serve: remove it from gateway/ec2/authz.go", action) + } +} diff --git a/spinifex/gateway/ec2_authz_test.go b/spinifex/gateway/ec2_authz_test.go new file mode 100644 index 000000000..96828d0c6 --- /dev/null +++ b/spinifex/gateway/ec2_authz_test.go @@ -0,0 +1,244 @@ +//test:in-package — drives EC2_Request through the gateway's unexported test +// helpers (setupEC2Request, policyMockIAMService) and auth context keys. + +package gateway + +import ( + "net/http/httptest" + "strconv" + "strings" + "testing" + + "github.com/mulgadc/spinifex/spinifex/awsec2query" + "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" +) + +const ( + authzAccountID = "123456789012" + authzRegion = "ap-southeast-2" +) + +// scopedPolicyGateway serves one policy document to every principal. +func scopedPolicyGateway(statements ...handlers_iam.Statement) *GatewayConfig { + docs := []handlers_iam.PolicyDocument{{Version: "2012-10-17", Statement: statements}} + return &GatewayConfig{ + DisableLogging: true, + Region: authzRegion, + IAMService: &policyMockIAMService{ + getUserPoliciesFn: func(_, _ string) ([]handlers_iam.PolicyDocument, error) { return docs, nil }, + getRolePoliciesFn: func(_, _ string) ([]handlers_iam.PolicyDocument, error) { return docs, nil }, + }, + } +} + +func statement(effect, action, resource string) handlers_iam.Statement { + return handlers_iam.Statement{ + Effect: effect, + Action: handlers_iam.StringOrArr{action}, + Resource: handlers_iam.StringOrArr{resource}, + } +} + +// dispatchEC2 drives the gateway with no NATS connection. A permitted request +// therefore reaches the handler and fails there, which is what proves the policy +// check ran ahead of the resource existing at all. +func dispatchEC2(t *testing.T, gw *GatewayConfig, body string) error { + t.Helper() + return gw.EC2_Request(httptest.NewRecorder(), setupEC2Request(body, authzAccountID)) +} + +func assertDenied(t *testing.T, err error) { + t.Helper() + require.Error(t, err) + assert.Equal(t, awserrors.ErrorAccessDenied, err.Error()) +} + +// assertPermitted asserts the policy gate passed. The request then fails on the +// absent NATS connection rather than on authorization, so a denial cannot hide +// behind a not-found. +func assertPermitted(t *testing.T, err error) { + t.Helper() + require.Error(t, err) + assert.Equal(t, awserrors.ErrorServerInternal, err.Error()) +} + +// TestEC2Request_ScopedDenyFires is the bypass this work closes. An operator +// fences a production instance; before the resolver the fence was inert and +// TerminateInstances against it was permitted with nothing logged. +func TestEC2Request_ScopedDenyFires(t *testing.T) { + gw := scopedPolicyGateway( + statement("Allow", "ec2:*", "*"), + statement("Deny", "ec2:TerminateInstances", "arn:aws:ec2:*:*:instance/i-prod"), + ) + + assertDenied(t, dispatchEC2(t, gw, "Action=TerminateInstances&InstanceId.1=i-prod")) + assertPermitted(t, dispatchEC2(t, gw, "Action=TerminateInstances&InstanceId.1=i-dev")) +} + +// TestEC2Request_ScopedAllowGrants is the other half: a least-privilege policy +// used to deny everything, so the only working policy shape was Resource "*". +func TestEC2Request_ScopedAllowGrants(t *testing.T) { + gw := scopedPolicyGateway( + statement("Allow", "ec2:TerminateInstances", "arn:aws:ec2:*:*:instance/i-dev"), + ) + + assertPermitted(t, dispatchEC2(t, gw, "Action=TerminateInstances&InstanceId.1=i-dev")) + assertDenied(t, dispatchEC2(t, gw, "Action=TerminateInstances&InstanceId.1=i-prod")) +} + +// TestEC2Request_DenyOneMemberFailsTheBatch pins the AWS combining rule: a batch +// containing one fenced instance fails whole rather than terminating the rest. +func TestEC2Request_DenyOneMemberFailsTheBatch(t *testing.T) { + gw := scopedPolicyGateway( + statement("Allow", "ec2:*", "*"), + statement("Deny", "ec2:TerminateInstances", "arn:aws:ec2:*:*:instance/i-prod"), + ) + + assertDenied(t, dispatchEC2(t, gw, + "Action=TerminateInstances&InstanceId.1=i-dev&InstanceId.2=i-prod&InstanceId.3=i-other")) + assertPermitted(t, dispatchEC2(t, gw, + "Action=TerminateInstances&InstanceId.1=i-dev&InstanceId.2=i-other")) +} + +// TestEC2Request_MultiResourceParity covers a resource beyond the primary one. +// A tenant constraining launches by instance would otherwise have the AMI and +// subnet unchecked, which is a smaller instance of the same bug. +func TestEC2Request_MultiResourceParity(t *testing.T) { + gw := scopedPolicyGateway( + statement("Allow", "ec2:*", "*"), + statement("Deny", "ec2:RunInstances", "arn:aws:ec2:*:*:image/ami-restricted"), + statement("Deny", "ec2:RunInstances", "arn:aws:ec2:*:*:subnet/subnet-restricted"), + ) + + assertDenied(t, dispatchEC2(t, gw, "Action=RunInstances&ImageId=ami-restricted&MinCount=1&MaxCount=1")) + assertDenied(t, dispatchEC2(t, gw, + "Action=RunInstances&ImageId=ami-allowed&NetworkInterface.1.SubnetId=subnet-restricted&MinCount=1&MaxCount=1")) + assertPermitted(t, dispatchEC2(t, gw, "Action=RunInstances&ImageId=ami-allowed&MinCount=1&MaxCount=1")) +} + +func TestEC2Request_UsesCanonicalParsedResources(t *testing.T) { + t.Run("lower camel location name", func(t *testing.T) { + gw := scopedPolicyGateway( + statement("Allow", "ec2:*", "*"), + statement("Deny", "ec2:ModifyInstanceAttribute", "arn:aws:ec2:*:*:instance/i-prod"), + ) + assertDenied(t, dispatchEC2(t, gw, "Action=ModifyInstanceAttribute&instanceId=i-prod")) + }) + + t.Run("location name list wrapper", func(t *testing.T) { + gw := scopedPolicyGateway( + statement("Allow", "ec2:*", "*"), + statement("Deny", "ec2:TerminateInstances", "arn:aws:ec2:*:*:instance/i-prod"), + ) + assertDenied(t, dispatchEC2(t, gw, "Action=TerminateInstances&InstanceId.InstanceId.1=i-prod")) + }) + + t.Run("handler field precedence", func(t *testing.T) { + gw := scopedPolicyGateway( + statement("Allow", "ec2:*", "*"), + statement("Deny", "ec2:DeleteKeyPair", "arn:aws:ec2:*:*:key-pair/key-prod"), + ) + assertDenied(t, dispatchEC2(t, gw, "Action=DeleteKeyPair&KeyName=dev&KeyPairId=key-prod")) + }) +} + +func TestEC2Request_CreateRouteChecksNATGateway(t *testing.T) { + gw := scopedPolicyGateway( + statement("Allow", "ec2:*", "*"), + statement("Deny", "ec2:CreateRoute", "arn:aws:ec2:*:*:natgateway/nat-prod"), + ) + assertDenied(t, dispatchEC2(t, gw, "Action=CreateRoute&RouteTableId=rtb-dev&NatGatewayId=nat-prod")) +} + +func TestEC2Request_RequestSpotInstancesChecksLaunchResources(t *testing.T) { + gw := scopedPolicyGateway( + statement("Allow", "ec2:*", "*"), + statement("Deny", "ec2:RequestSpotInstances", "arn:aws:ec2:*:*:subnet/subnet-prod"), + ) + body := "Action=RequestSpotInstances&LaunchSpecification.ImageId=ami-1&" + + "LaunchSpecification.InstanceType=t3.micro&LaunchSpecification.SubnetId=subnet-prod" + assertDenied(t, dispatchEC2(t, gw, body)) +} + +func TestEC2Request_RejectsOversizedResourceList(t *testing.T) { + var body strings.Builder + body.WriteString("Action=TerminateInstances") + for i := 1; i <= awsec2query.MaxSliceLen+1; i++ { + body.WriteString("&InstanceId.") + body.WriteString(strconv.Itoa(i)) + body.WriteString("=i-") + body.WriteString(strconv.Itoa(i)) + } + + gw := scopedPolicyGateway(statement("Allow", "ec2:*", "*")) + err := dispatchEC2(t, gw, body.String()) + require.Error(t, err) + assert.Equal(t, awserrors.ErrorMalformedQueryString, err.Error()) +} + +// TestEC2Request_AccountWideAllowIsUnaffected is the property the remaining +// per-service beads rest on: passing a real ARN where "*" was passed cannot +// withdraw access that works today. +func TestEC2Request_AccountWideAllowIsUnaffected(t *testing.T) { + for _, policy := range []handlers_iam.Statement{ + statement("Allow", "ec2:*", "*"), + statement("Allow", "*", "*"), + } { + gw := scopedPolicyGateway(policy) + for _, body := range []string{ + "Action=TerminateInstances&InstanceId.1=i-abc", + "Action=RunInstances&ImageId=ami-1&SubnetId=subnet-1&KeyName=k1", + "Action=CreateTags&ResourceId.1=i-abc&Tag.1.Key=Name&Tag.1.Value=x", + "Action=CreateTags&ResourceId.1=nope-abc&Tag.1.Key=Name&Tag.1.Value=x", + "Action=DeleteVolume&VolumeId=vol-1", + "Action=DescribeInstances", + } { + t.Run(body, func(t *testing.T) { + assertPermitted(t, dispatchEC2(t, gw, body)) + }) + } + } +} + +// TestEC2Request_MissingIdentifierStaysAValidationFault: the gate must not turn +// a malformed request into a denial. With an account-wide grant the absent +// identifier resolves to "*" and the handler reports its own fault. +func TestEC2Request_MissingIdentifierStaysAValidationFault(t *testing.T) { + gw := scopedPolicyGateway(statement("Allow", "ec2:*", "*")) + + assertPermitted(t, dispatchEC2(t, gw, "Action=TerminateInstances")) + assertPermitted(t, dispatchEC2(t, gw, "Action=DeleteVolume")) +} + +// TestEC2Request_IdentifierIsAValueNotAPattern: an id of literally * builds an +// ARN ending /*, which neither matches a scoped Deny on a real instance nor +// widens a grant to one. +func TestEC2Request_IdentifierIsAValueNotAPattern(t *testing.T) { + fenced := scopedPolicyGateway( + statement("Allow", "ec2:*", "*"), + statement("Deny", "ec2:TerminateInstances", "arn:aws:ec2:*:*:instance/i-prod"), + ) + assertPermitted(t, dispatchEC2(t, fenced, "Action=TerminateInstances&InstanceId.1=*")) + + // An id containing / is not truncated on the way to the evaluator. The + // reverted STS work built an ARN from the segment after the final /, so a + // Deny on i-prod fenced i-prod/admin and a grant fenced the wrong object. + assertPermitted(t, dispatchEC2(t, fenced, "Action=TerminateInstances&InstanceId.1=i-prod/admin")) + assertDenied(t, dispatchEC2(t, fenced, "Action=TerminateInstances&InstanceId.1=i-prod")) +} + +// TestEC2Request_ARNIsBuiltFromTheNodeRegion: the credential-scope region is +// caller-supplied and never validated, so an ARN built from it would let a +// caller sign for another region and slide out from under a Deny scoped to the +// real one. +func TestEC2Request_ARNIsBuiltFromTheNodeRegion(t *testing.T) { + gw := scopedPolicyGateway( + statement("Allow", "ec2:*", "*"), + statement("Deny", "ec2:TerminateInstances", "arn:aws:ec2:"+authzRegion+":*:instance/i-prod"), + ) + + assertDenied(t, dispatchEC2(t, gw, "Action=TerminateInstances&InstanceId.1=i-prod")) +} diff --git a/spinifex/gateway/ec2_emptyresult_test.go b/spinifex/gateway/ec2_emptyresult_test.go index 6adfca9ec..9c8e4970b 100644 --- a/spinifex/gateway/ec2_emptyresult_test.go +++ b/spinifex/gateway/ec2_emptyresult_test.go @@ -2,7 +2,6 @@ package gateway import ( "context" - "net/http" "regexp" "strings" "testing" @@ -22,7 +21,9 @@ func render[In any](t *testing.T, action string, out any) string { h := ec2Handler(func(_ context.Context, _ *In, _ *GatewayConfig, _ string) (any, error) { return out, nil }) - xmlOutput, err := h(action, map[string]string{}, &GatewayConfig{}, "acct-123", (*http.Request)(nil)) + input, err := h.parse(map[string]string{}) + require.NoError(t, err) + xmlOutput, err := h.dispatch(action, input, &GatewayConfig{}, "acct-123", nil) require.NoError(t, err) return string(xmlOutput) } diff --git a/spinifex/gateway/gateway.go b/spinifex/gateway/gateway.go index 22aa33137..528765799 100644 --- a/spinifex/gateway/gateway.go +++ b/spinifex/gateway/gateway.go @@ -515,7 +515,10 @@ func isNATSTransient(err error) bool { errors.Is(err, nats.ErrNoStreamResponse)) } -// checkPolicy evaluates IAM policies against resource "*". +// checkPolicy evaluates IAM policies against the literal resource "*", so it +// grants account-wide: a caller's resource-scoped statements do not participate +// at all, neither a Deny that fences a resource nor an Allow that names one. +// Prefer checkPolicyResources with the request's real ARNs. func (gw *GatewayConfig) checkPolicy(r *http.Request, service, action string) error { return gw.checkPolicyResources(r, service, action, []string{"*"}) } diff --git a/spinifex/gateway/gateway_test.go b/spinifex/gateway/gateway_test.go index 1768d9983..3f50cdcbf 100644 --- a/spinifex/gateway/gateway_test.go +++ b/spinifex/gateway/gateway_test.go @@ -1343,9 +1343,8 @@ func TestImportKeyPair_Base64PaddingWorkaround(t *testing.T) { "PublicKeyMaterial": "c3NoLXJzYSBBQUFBQjNOemFDMXljMkVBQUFBREFRQUJBQUFCZ1FD%3D%3D", } - gw := &GatewayConfig{DisableLogging: true, NATSConn: nil, IAMService: allowAllIAMService()} - // NATS is nil so the handler errors, but PublicKeyMaterial is modified before that. - _, _ = handler("ImportKeyPair", q, gw, "123456789012", nil) + _, err := handler.parse(q) + require.NoError(t, err) assert.True(t, strings.HasSuffix(q["PublicKeyMaterial"], "=="), "Expected PublicKeyMaterial to end with == but got: %s", q["PublicKeyMaterial"]) diff --git a/spinifex/gateway/policy/evaluator_test.go b/spinifex/gateway/policy/evaluator_test.go index 943e72e31..7fcb1d57b 100644 --- a/spinifex/gateway/policy/evaluator_test.go +++ b/spinifex/gateway/policy/evaluator_test.go @@ -106,3 +106,68 @@ func TestIAMAction(t *testing.T) { t.Fatalf("IAMAction(ec2, RunInstances) = %q, want %q", got, "ec2:RunInstances") } } + +// --- Resource scoping --- + +// TestEvaluate_ResourceScopedStatementsNeedARealARN pins why the gateway must +// pass the request's ARN rather than "*". The evaluator matches the statement's +// Resource as a pattern against the request's resource as a value, so a scoped +// statement never matches "*" in either direction: a Deny fails open and an +// Allow fails closed. +func TestEvaluate_ResourceScopedStatementsNeedARealARN(t *testing.T) { + const prod = "arn:aws:ec2:ap-southeast-2:123456789012:instance/i-prod" + const dev = "arn:aws:ec2:ap-southeast-2:123456789012:instance/i-dev" + + fenced := []handlers_iam.PolicyDocument{ + doc("Allow", "ec2:*", "*"), + doc("Deny", "ec2:TerminateInstances", "arn:aws:ec2:*:*:instance/i-prod"), + } + if got := iampolicy.EvaluateWithKeys("ec2:TerminateInstances", "*", fenced, nil); got != iampolicy.Allow { + t.Errorf("fenced policy against %q: got %v, want Allow — the guardrail is inert", "*", got) + } + if got := iampolicy.EvaluateWithKeys("ec2:TerminateInstances", prod, fenced, nil); got != iampolicy.Deny { + t.Errorf("fenced policy against the fenced instance: got %v, want Deny", got) + } + if got := iampolicy.EvaluateWithKeys("ec2:TerminateInstances", dev, fenced, nil); got != iampolicy.Allow { + t.Errorf("fenced policy against an unfenced instance: got %v, want Allow", got) + } + + scopedAllow := []handlers_iam.PolicyDocument{ + doc("Allow", "ec2:*", "arn:aws:ec2:*:*:instance/i-dev"), + } + if got := iampolicy.EvaluateWithKeys("ec2:TerminateInstances", "*", scopedAllow, nil); got != iampolicy.Deny { + t.Errorf("scoped Allow against %q: got %v, want Deny — least privilege is unexpressible", "*", got) + } + if got := iampolicy.EvaluateWithKeys("ec2:TerminateInstances", dev, scopedAllow, nil); got != iampolicy.Allow { + t.Errorf("scoped Allow against the named instance: got %v, want Allow", got) + } + if got := iampolicy.EvaluateWithKeys("ec2:TerminateInstances", prod, scopedAllow, nil); got != iampolicy.Deny { + t.Errorf("scoped Allow against a sibling instance: got %v, want Deny", got) + } +} + +// TestEvaluate_PassingARealARNCannotWithdrawAccess is the property the +// per-service rollout rests on. Every policy that functions today carries +// Resource "*" for the action, and "*" as a pattern matches any ARN value, so +// handing the evaluator a real ARN cannot turn an Allow into a Deny — not even +// a malformed one, which bounds a resolver bug to the newly-scoped statements. +func TestEvaluate_PassingARealARNCannotWithdrawAccess(t *testing.T) { + resources := []string{ + "*", + "arn:aws:ec2:ap-southeast-2:123456789012:instance/i-abc", + "arn:aws:ec2:ap-southeast-2:123456789012:instance/*", + "arn:aws:ec2:ap-southeast-2:123456789012:instance/i-abc/admin", + "not-an-arn", + } + policies := [][]handlers_iam.PolicyDocument{ + {doc("Allow", "ec2:*", "*")}, + {doc("Allow", "*", "*")}, + } + for _, p := range policies { + for _, resource := range resources { + if got := iampolicy.EvaluateWithKeys("ec2:RunInstances", resource, p, nil); got != iampolicy.Allow { + t.Errorf("account-wide Allow against %q: got %v, want Allow", resource, got) + } + } + } +}