diff --git a/spinifex/arn/ec2.go b/spinifex/arn/ec2.go index 3ea788f37..0ae2a3ebf 100644 --- a/spinifex/arn/ec2.go +++ b/spinifex/arn/ec2.go @@ -21,6 +21,7 @@ const ( EC2VPC EC2ResourceType = "vpc" EC2Subnet EC2ResourceType = "subnet" EC2SecurityGroup EC2ResourceType = "security-group" + EC2SecurityGroupRule EC2ResourceType = "security-group-rule" EC2RouteTable EC2ResourceType = "route-table" EC2InternetGateway EC2ResourceType = "internet-gateway" EC2EgressOnlyInternetGateway EC2ResourceType = "egress-only-internet-gateway" @@ -52,6 +53,7 @@ var ec2IDPrefixes = []struct { {"vpc-", EC2VPC}, {"subnet-", EC2Subnet}, {"sg-", EC2SecurityGroup}, + {"sgr-", EC2SecurityGroupRule}, {"rtb-", EC2RouteTable}, {"igw-", EC2InternetGateway}, {"eigw-", EC2EgressOnlyInternetGateway}, diff --git a/spinifex/arn/ec2_test.go b/spinifex/arn/ec2_test.go index 229fc63fd..d4a7cdfdc 100644 --- a/spinifex/arn/ec2_test.go +++ b/spinifex/arn/ec2_test.go @@ -20,6 +20,7 @@ func TestEC2TypeForID(t *testing.T) { {"vpc-abc123", arn.EC2VPC, true}, {"subnet-abc123", arn.EC2Subnet, true}, {"sg-abc123", arn.EC2SecurityGroup, true}, + {"sgr-abc123", arn.EC2SecurityGroupRule, true}, {"rtb-abc123", arn.EC2RouteTable, true}, {"igw-abc123", arn.EC2InternetGateway, true}, {"eigw-abc123", arn.EC2EgressOnlyInternetGateway, true}, diff --git a/spinifex/daemon/daemon_handlers_tag.go b/spinifex/daemon/daemon_handlers_tag.go index 2f0434a38..2bc2b5edb 100644 --- a/spinifex/daemon/daemon_handlers_tag.go +++ b/spinifex/daemon/daemon_handlers_tag.go @@ -30,7 +30,7 @@ type recordTagMirror interface { } // recordTagMirrors collects the initialized services owning centrally-stored -// records: vpc covers vpc/subnet/sg/eni, the rest own their prefix. +// records: vpc covers vpc/subnet/sg/sgr/eni, the rest own their prefix. func (d *Daemon) recordTagMirrors() []recordTagMirror { var mirrors []recordTagMirror if d.vpcService != nil { diff --git a/spinifex/handlers/ec2/vpc/security_group.go b/spinifex/handlers/ec2/vpc/security_group.go index 165ebcc37..7ab11d8a4 100644 --- a/spinifex/handlers/ec2/vpc/security_group.go +++ b/spinifex/handlers/ec2/vpc/security_group.go @@ -6,6 +6,7 @@ import ( "errors" "fmt" "log/slog" + "maps" "net" "regexp" "slices" @@ -29,6 +30,10 @@ var sgIDRegex = regexp.MustCompile(`^sg-[0-9a-f]{17}$`) // re-implementing the format check. var SGRuleIDRegex = regexp.MustCompile(`^sgr-[0-9a-f]{17}$`) +// allProtocols is the IpProtocol value naming every protocol, which is also +// the port value AWS reports for such a rule. +const allProtocols = "-1" + // canonicalIPProtocols are the only values the OVN ACL builder can express. // Anything else must be rejected: the builder emits no L4 predicate for an // unknown protocol, which widens the rule to every IP protocol. @@ -129,6 +134,10 @@ type SGRule struct { // so revoke/duplicate matching ignores it. The AWS Load Balancer Controller // tags the node-SG rules it manages here and revokes them by this value. Description string `json:"description,omitempty"` + // Tags are excluded from sgRuleKey for the same reason as Description: two + // rules with identical permissions are the same rule whatever they are + // labelled. + Tags map[string]string `json:"tags,omitempty"` } // SecurityGroupRecord represents a stored security group. @@ -811,13 +820,11 @@ func (s *VPCServiceImpl) DescribeSecurityGroupRules(ctx context.Context, input * } // sgRuleMatchesFilters applies the DescribeSecurityGroupRules filter set to a -// single rule. Rule-level tags are not yet persisted, so tag:* and tag-key -// filters always return false. +// single rule. func sgRuleMatchesFilters(record *SecurityGroupRecord, rule SGRule, filters map[string][]string) bool { for name, values := range filters { if strings.HasPrefix(name, "tag:") { - // rule.Tags is not yet populated; any tag:* filter excludes every rule. - return false + continue } switch name { case "group-id": @@ -829,8 +836,9 @@ func sgRuleMatchesFilters(record *SecurityGroupRecord, rule SGRule, filters map[ return false } case "tag-key": - // rule.Tags is not yet populated; tag-key excludes every rule. - return false + if !sgRuleMatchesTagKey(rule.Tags, values) { + return false + } default: // Unreachable: ParseFilters rejects names not in the valid set. // Logs an error if a future map entry lacks a matching case. @@ -838,21 +846,121 @@ func sgRuleMatchesFilters(record *SecurityGroupRecord, rule SGRule, filters map[ return false } } - return true + return filterutil.MatchesTags(filters, rule.Tags) +} + +// updateSGRuleTags applies mut to the tags of the rule named by ruleID, which +// lives inside its parent group's record rather than a record of its own. The +// parent is found by scanning the account's groups: a rule id carries no +// pointer back to the group holding it. +func (s *VPCServiceImpl) updateSGRuleTags(ctx context.Context, accountID, ruleID string, mut func(map[string]string)) error { + prefix := accountID + "." + keys, err := s.sgKV.Keys(ctx) + if err != nil { + if errors.Is(err, jetstream.ErrNoKeysFound) { + return nil + } + slog.ErrorContext(ctx, "updateSGRuleTags: key listing failed", "ruleId", ruleID, "err", err) + return errors.New(awserrors.ErrorServerInternal) + } + + for _, key := range keys { + if key == utils.VersionKey || !strings.HasPrefix(key, prefix) { + continue + } + entry, err := s.sgKV.Get(ctx, key) + if err != nil { + slog.ErrorContext(ctx, "updateSGRuleTags: SG read failed", "key", key, "err", err) + return errors.New(awserrors.ErrorServerInternal) + } + var record SecurityGroupRecord + if err := json.Unmarshal(entry.Value(), &record); err != nil { + slog.ErrorContext(ctx, "updateSGRuleTags: SG unmarshal failed", "key", key, "err", err) + return errors.New(awserrors.ErrorServerInternal) + } + + rule := findSGRuleByID(record.IngressRules, ruleID) + if rule == nil { + rule = findSGRuleByID(record.EgressRules, ruleID) + } + if rule == nil { + continue + } + if rule.Tags == nil { + rule.Tags = map[string]string{} + } + mut(rule.Tags) + + data, err := json.Marshal(record) + if err != nil { + return fmt.Errorf("failed to marshal security group record: %w", err) + } + // Compare-and-swap on the revision read above: a concurrent authorize on + // the same group would otherwise lose its rule to this write. + if _, err := s.sgKV.Update(ctx, key, data, entry.Revision()); err != nil { + slog.ErrorContext(ctx, "updateSGRuleTags: SG write failed", "key", key, "ruleId", ruleID, "err", err) + return errors.New(awserrors.ErrorServerInternal) + } + return nil + } + return nil +} + +// findSGRuleByID returns a pointer into rules so a caller can mutate the stored +// rule in place. +func findSGRuleByID(rules []SGRule, ruleID string) *SGRule { + for i := range rules { + if rules[i].RuleId == ruleID { + return &rules[i] + } + } + return nil +} + +// sgRuleMatchesTagKey reports whether the rule carries any of the named tag +// keys, whatever their values. +func sgRuleMatchesTagKey(tags map[string]string, keys []string) bool { + for key := range tags { + if filterutil.MatchesAny(keys, key) { + return true + } + } + return false +} + +// applySGRuleTags stamps the security-group-rule TagSpecification onto every +// rule an authorize call creates. AWS tags the rules the call creates and no +// others, so this runs on the new rules alone. +func applySGRuleTags(rules []SGRule, specs []*ec2.TagSpecification) []SGRule { + tags := utils.ExtractTags(specs, ec2.ResourceTypeSecurityGroupRule) + if len(tags) == 0 { + return rules + } + for i := range rules { + rules[i].Tags = maps.Clone(tags) + } + return rules } // sgRuleToSecurityGroupRule flattens a stored SGRule into the AWS API shape. // accountID supplies GroupOwnerId and ReferencedGroupInfo.UserId; VpcId is // derived from the parent record (same-VPC references are enforced on write). func sgRuleToSecurityGroupRule(record *SecurityGroupRecord, rule SGRule, isEgress bool, accountID string) *ec2.SecurityGroupRule { + // An all-protocol rule has no ports to report. AWS answers -1 for both; + // reporting the stored zeroes offers a caller two ports it never sent. + fromPort, toPort := rule.FromPort, rule.ToPort + if rule.IpProtocol == allProtocols { + fromPort, toPort = -1, -1 + } out := &ec2.SecurityGroupRule{ SecurityGroupRuleId: aws.String(rule.RuleId), GroupId: aws.String(record.GroupId), GroupOwnerId: aws.String(accountID), IsEgress: aws.Bool(isEgress), IpProtocol: aws.String(rule.IpProtocol), - FromPort: aws.Int64(rule.FromPort), - ToPort: aws.Int64(rule.ToPort), + FromPort: aws.Int64(fromPort), + ToPort: aws.Int64(toPort), + Tags: utils.MapToEC2Tags(rule.Tags), } if rule.Description != "" { out.Description = aws.String(rule.Description) @@ -901,6 +1009,7 @@ func (s *VPCServiceImpl) AuthorizeSecurityGroupIngress(ctx context.Context, inpu slog.WarnContext(ctx, "AuthorizeSecurityGroupIngress: invalid rule", "groupId", groupId, "err", err) return nil, awserrors.Errorf(awserrors.ErrorInvalidParameterValue, "%s", err) } + newRules = applySGRuleTags(newRules, input.TagSpecifications) if err := s.validateSGRuleReferences(ctx, accountID, record.VpcId, newRules); err != nil { return nil, err } @@ -926,6 +1035,10 @@ func (s *VPCServiceImpl) AuthorizeSecurityGroupIngress(ctx context.Context, inpu return nil, errors.New(awserrors.ErrorServerInternal) } + for _, nr := range newRules { + s.projectRecordTags(ctx, accountID, nr.RuleId, nr.Tags) + } + slog.InfoContext(ctx, "AuthorizeSecurityGroupIngress completed", "groupId", groupId, "newRules", len(newRules), "accountID", accountID) if err := s.requestSGEvent("vpc.update-sg", SGEvent{ @@ -976,6 +1089,7 @@ func (s *VPCServiceImpl) AuthorizeSecurityGroupEgress(ctx context.Context, input slog.WarnContext(ctx, "AuthorizeSecurityGroupEgress: invalid rule", "groupId", groupId, "err", err) return nil, awserrors.Errorf(awserrors.ErrorInvalidParameterValue, "%s", err) } + newRules = applySGRuleTags(newRules, input.TagSpecifications) if err := s.validateSGRuleReferences(ctx, accountID, record.VpcId, newRules); err != nil { return nil, err } @@ -1001,6 +1115,10 @@ func (s *VPCServiceImpl) AuthorizeSecurityGroupEgress(ctx context.Context, input return nil, errors.New(awserrors.ErrorServerInternal) } + for _, nr := range newRules { + s.projectRecordTags(ctx, accountID, nr.RuleId, nr.Tags) + } + slog.InfoContext(ctx, "AuthorizeSecurityGroupEgress completed", "groupId", groupId, "newRules", len(newRules), "accountID", accountID) if err := s.requestSGEvent("vpc.update-sg", SGEvent{ @@ -1058,6 +1176,7 @@ func (s *VPCServiceImpl) RevokeSecurityGroupIngress(ctx context.Context, input * if err := assertRulesPresent(record.IngressRules, revokeRules); err != nil { return nil, err } + revokedIDs := removedSGRuleIDs(record.IngressRules, revokeRules) record.IngressRules = removeSGRules(record.IngressRules, revokeRules) data, err := json.Marshal(record) @@ -1068,6 +1187,10 @@ func (s *VPCServiceImpl) RevokeSecurityGroupIngress(ctx context.Context, input * return nil, errors.New(awserrors.ErrorServerInternal) } + for _, id := range revokedIDs { + s.clearRecordTags(ctx, accountID, id) + } + slog.InfoContext(ctx, "RevokeSecurityGroupIngress completed", "groupId", groupId, "revokedRules", len(revokeRules), "accountID", accountID) if err := s.requestSGEvent("vpc.update-sg", SGEvent{ @@ -1120,6 +1243,7 @@ func (s *VPCServiceImpl) RevokeSecurityGroupEgress(ctx context.Context, input *e if err := assertRulesPresent(record.EgressRules, revokeRules); err != nil { return nil, err } + revokedIDs := removedSGRuleIDs(record.EgressRules, revokeRules) record.EgressRules = removeSGRules(record.EgressRules, revokeRules) data, err := json.Marshal(record) @@ -1130,6 +1254,10 @@ func (s *VPCServiceImpl) RevokeSecurityGroupEgress(ctx context.Context, input *e return nil, errors.New(awserrors.ErrorServerInternal) } + for _, id := range revokedIDs { + s.clearRecordTags(ctx, accountID, id) + } + slog.InfoContext(ctx, "RevokeSecurityGroupEgress completed", "groupId", groupId, "revokedRules", len(revokeRules), "accountID", accountID) if err := s.requestSGEvent("vpc.update-sg", SGEvent{ @@ -1575,6 +1703,24 @@ func assertRulesPresent(existing, toRevoke []SGRule) error { return nil } +// removedSGRuleIDs names the stored rules a revoke will drop. A revoke request +// carries permissions rather than IDs, so the IDs whose tag entries have to go +// with them can only be read off the stored rules before they are removed. +func removedSGRuleIDs(existing, toRemove []SGRule) []string { + removeSet := make(map[string]bool, len(toRemove)) + for _, r := range toRemove { + removeSet[sgRuleKey(r)] = true + } + + var ids []string + for _, r := range existing { + if removeSet[sgRuleKey(r)] { + ids = append(ids, r.RuleId) + } + } + return ids +} + // removeSGRules removes matching rules from the existing set. func removeSGRules(existing, toRemove []SGRule) []SGRule { removeSet := make(map[string]bool) diff --git a/spinifex/handlers/ec2/vpc/security_group_test.go b/spinifex/handlers/ec2/vpc/security_group_test.go index ee1c484d1..ddfb50863 100644 --- a/spinifex/handlers/ec2/vpc/security_group_test.go +++ b/spinifex/handlers/ec2/vpc/security_group_test.go @@ -10,6 +10,7 @@ import ( "github.com/aws/aws-sdk-go/aws" "github.com/aws/aws-sdk-go/service/ec2" "github.com/mulgadc/spinifex/spinifex/awserrors" + "github.com/mulgadc/spinifex/spinifex/filterutil" "github.com/mulgadc/spinifex/spinifex/testutil" "github.com/mulgadc/spinifex/spinifex/utils" "github.com/nats-io/nats.go" @@ -2362,3 +2363,141 @@ func TestSecurityGroupRule_DescriptionRoundTrips(t *testing.T) { _, err = svc.DeleteSecurityGroup(context.Background(), &ec2.DeleteSecurityGroupInput{GroupId: aws.String(backendSG)}, testAccountID) require.NoError(t, err, "backend SG must delete once its node-SG reference is revoked") } + +func TestAuthorizeSecurityGroupIngress_StoresRuleTags(t *testing.T) { + t.Parallel() + svc := setupTestVPCService(t) + vpcID := createTestVPC(t, svc, "10.0.0.0/16") + sgID := createTestSG(t, svc, vpcID, "rule-tags-sg") + + out, err := svc.AuthorizeSecurityGroupIngress(context.Background(), &ec2.AuthorizeSecurityGroupIngressInput{ + GroupId: aws.String(sgID), + IpPermissions: []*ec2.IpPermission{{ + IpProtocol: aws.String("tcp"), + FromPort: aws.Int64(80), + ToPort: aws.Int64(80), + IpRanges: []*ec2.IpRange{{CidrIp: aws.String("192.168.1.33/32")}}, + }}, + TagSpecifications: []*ec2.TagSpecification{{ + ResourceType: aws.String(ec2.ResourceTypeSecurityGroupRule), + Tags: []*ec2.Tag{{Key: aws.String("terraform-aws-modules"), Value: aws.String("alb")}}, + }}, + }, testAccountID) + require.NoError(t, err) + require.Len(t, out.SecurityGroupRules, 1) + ruleID := aws.StringValue(out.SecurityGroupRules[0].SecurityGroupRuleId) + assert.Equal(t, map[string]string{"terraform-aws-modules": "alb"}, + filterutil.EC2TagsToMap(out.SecurityGroupRules[0].Tags), "authorize must echo the tags it stored") + + desc, err := svc.DescribeSecurityGroupRules(context.Background(), &ec2.DescribeSecurityGroupRulesInput{ + SecurityGroupRuleIds: []*string{aws.String(ruleID)}, + }, testAccountID) + require.NoError(t, err) + require.Len(t, desc.SecurityGroupRules, 1) + assert.Equal(t, map[string]string{"terraform-aws-modules": "alb"}, + filterutil.EC2TagsToMap(desc.SecurityGroupRules[0].Tags)) +} + +func TestDescribeSecurityGroupRules_FiltersByTag(t *testing.T) { + t.Parallel() + svc := setupTestVPCService(t) + vpcID := createTestVPC(t, svc, "10.0.0.0/16") + sgID := createTestSG(t, svc, vpcID, "rule-tag-filter-sg") + + tagged, err := svc.AuthorizeSecurityGroupIngress(context.Background(), &ec2.AuthorizeSecurityGroupIngressInput{ + GroupId: aws.String(sgID), + IpPermissions: []*ec2.IpPermission{{ + IpProtocol: aws.String("tcp"), FromPort: aws.Int64(443), ToPort: aws.Int64(443), + IpRanges: []*ec2.IpRange{{CidrIp: aws.String("10.0.0.0/8")}}, + }}, + TagSpecifications: []*ec2.TagSpecification{{ + ResourceType: aws.String(ec2.ResourceTypeSecurityGroupRule), + Tags: []*ec2.Tag{{Key: aws.String("owner"), Value: aws.String("alb")}}, + }}, + }, testAccountID) + require.NoError(t, err) + taggedID := aws.StringValue(tagged.SecurityGroupRules[0].SecurityGroupRuleId) + + _, err = svc.AuthorizeSecurityGroupIngress(context.Background(), &ec2.AuthorizeSecurityGroupIngressInput{ + GroupId: aws.String(sgID), + IpPermissions: []*ec2.IpPermission{{ + IpProtocol: aws.String("tcp"), FromPort: aws.Int64(8080), ToPort: aws.Int64(8080), + IpRanges: []*ec2.IpRange{{CidrIp: aws.String("10.0.0.0/8")}}, + }}, + }, testAccountID) + require.NoError(t, err) + + byValue, err := svc.DescribeSecurityGroupRules(context.Background(), &ec2.DescribeSecurityGroupRulesInput{ + Filters: []*ec2.Filter{{Name: aws.String("tag:owner"), Values: []*string{aws.String("alb")}}}, + }, testAccountID) + require.NoError(t, err) + require.Len(t, byValue.SecurityGroupRules, 1) + assert.Equal(t, taggedID, aws.StringValue(byValue.SecurityGroupRules[0].SecurityGroupRuleId)) + + byKey, err := svc.DescribeSecurityGroupRules(context.Background(), &ec2.DescribeSecurityGroupRulesInput{ + Filters: []*ec2.Filter{{Name: aws.String("tag-key"), Values: []*string{aws.String("owner")}}}, + }, testAccountID) + require.NoError(t, err) + require.Len(t, byKey.SecurityGroupRules, 1) + assert.Equal(t, taggedID, aws.StringValue(byKey.SecurityGroupRules[0].SecurityGroupRuleId)) +} + +func TestDescribeSecurityGroupRules_AllProtocolReportsMinusOnePorts(t *testing.T) { + t.Parallel() + svc := setupTestVPCService(t) + vpcID := createTestVPC(t, svc, "10.0.0.0/16") + sgID := createTestSG(t, svc, vpcID, "all-proto-sg") + + out, err := svc.AuthorizeSecurityGroupEgress(context.Background(), &ec2.AuthorizeSecurityGroupEgressInput{ + GroupId: aws.String(sgID), + IpPermissions: []*ec2.IpPermission{{ + IpProtocol: aws.String("-1"), + IpRanges: []*ec2.IpRange{{CidrIp: aws.String("10.1.0.0/16")}}, + }}, + }, testAccountID) + require.NoError(t, err) + require.Len(t, out.SecurityGroupRules, 1) + assert.Equal(t, int64(-1), aws.Int64Value(out.SecurityGroupRules[0].FromPort)) + assert.Equal(t, int64(-1), aws.Int64Value(out.SecurityGroupRules[0].ToPort)) +} + +func TestApplyRecordTags_ReachesASecurityGroupRule(t *testing.T) { + t.Parallel() + svc := setupTestVPCService(t) + vpcID := createTestVPC(t, svc, "10.0.0.0/16") + sgID := createTestSG(t, svc, vpcID, "rule-createtags-sg") + + out, err := svc.AuthorizeSecurityGroupIngress(context.Background(), &ec2.AuthorizeSecurityGroupIngressInput{ + GroupId: aws.String(sgID), + IpPermissions: []*ec2.IpPermission{{ + IpProtocol: aws.String("tcp"), FromPort: aws.Int64(22), ToPort: aws.Int64(22), + IpRanges: []*ec2.IpRange{{CidrIp: aws.String("192.168.1.33/32")}}, + }}, + }, testAccountID) + require.NoError(t, err) + ruleID := aws.StringValue(out.SecurityGroupRules[0].SecurityGroupRuleId) + + require.NoError(t, svc.ApplyRecordTags(&ec2.CreateTagsInput{ + Resources: []*string{aws.String(ruleID)}, + Tags: []*ec2.Tag{{Key: aws.String("Name"), Value: aws.String("ssh")}}, + }, testAccountID)) + + desc, err := svc.DescribeSecurityGroupRules(context.Background(), &ec2.DescribeSecurityGroupRulesInput{ + SecurityGroupRuleIds: []*string{aws.String(ruleID)}, + }, testAccountID) + require.NoError(t, err) + require.Len(t, desc.SecurityGroupRules, 1) + assert.Equal(t, map[string]string{"Name": "ssh"}, + filterutil.EC2TagsToMap(desc.SecurityGroupRules[0].Tags)) + + require.NoError(t, svc.RemoveRecordTags(&ec2.DeleteTagsInput{ + Resources: []*string{aws.String(ruleID)}, + Tags: []*ec2.Tag{{Key: aws.String("Name")}}, + }, testAccountID)) + + desc, err = svc.DescribeSecurityGroupRules(context.Background(), &ec2.DescribeSecurityGroupRulesInput{ + SecurityGroupRuleIds: []*string{aws.String(ruleID)}, + }, testAccountID) + require.NoError(t, err) + assert.Empty(t, desc.SecurityGroupRules[0].Tags) +} diff --git a/spinifex/handlers/ec2/vpc/service_impl.go b/spinifex/handlers/ec2/vpc/service_impl.go index 1417de95a..ece466d70 100644 --- a/spinifex/handlers/ec2/vpc/service_impl.go +++ b/spinifex/handlers/ec2/vpc/service_impl.go @@ -1107,10 +1107,13 @@ func (s *VPCServiceImpl) clearRecordTags(ctx context.Context, accountID, resourc } } -// updateRecordTags applies mut to the tag map of the subnet-, vpc-, sg-, or -// eni-scoped record identified by resourceID. Other resource ids are a no-op. +// updateRecordTags applies mut to the tag map of the subnet-, vpc-, sg-, sgr-, +// or eni-scoped record identified by resourceID. Other resource ids are a +// no-op. func (s *VPCServiceImpl) updateRecordTags(ctx context.Context, accountID, resourceID string, mut func(map[string]string)) error { switch { + case strings.HasPrefix(resourceID, "sgr-"): + return s.updateSGRuleTags(ctx, accountID, resourceID, mut) case strings.HasPrefix(resourceID, "subnet-"): return utils.UpdateKVRecordTags(ctx, s.subnetKV, accountID, resourceID, func(r *SubnetRecord) { if r.Tags == nil {