Skip to content

Fix secgroups targeting other secgroups in projects #352

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
bddvlpr wants to merge 1 commit into
base: main
Choose a base branch
from
+225 −8
Open

Fix secgroups targeting other secgroups in projects #352

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
83 changes: 78 additions & 5 deletions cloudstack/resource_cloudstack_security_group_rule.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -33,9 +33,12 @@ import (
)

type authorizeSecurityGroupParams interface {
SetAccount(string)
SetCidrlist([]string)
SetDomainid(string)
SetIcmptype(int)
SetIcmpcode(int)
SetProjectid(string)
SetStartport(int)
SetEndport(int)
SetProtocol(string)
Expand Down Expand Up @@ -155,6 +158,14 @@ func createSecurityGroupRules(d *schema.ResourceData, meta interface{}, rules *s
cs := meta.(*cloudstack.CloudStackClient)
var errs *multierror.Error

sg, _, err := cs.SecurityGroup.GetSecurityGroupByID(
d.Id(),
cloudstack.WithProject(d.Get("project").(string)),
)
if err != nil {
return err
}

var wg sync.WaitGroup
wg.Add(nrs.Len())

Expand Down Expand Up @@ -186,6 +197,7 @@ func createSecurityGroupRules(d *schema.ResourceData, meta interface{}, rules *s
}

p.SetSecuritygroupid(d.Id())
setSecurityGroupRuleOwnership(p, sg)
p.SetCidrlist([]string{cidr.(string)})

// Create a single rule
Expand All @@ -198,10 +210,12 @@ func createSecurityGroupRules(d *schema.ResourceData, meta interface{}, rules *s

if usgList, ok := rule["user_security_group_list"].(*schema.Set); ok && usgList.Len() > 0 {
for _, usg := range usgList.List() {
sg, _, err := cs.SecurityGroup.GetSecurityGroupByName(
usg.(string),
cloudstack.WithProject(d.Get("project").(string)),
)
sourceSG, err := getSecurityGroupRuleSourceGroup(cs, sg, usg.(string))
if err != nil {
errs = multierror.Append(errs, err)
continue
}
sourceAccount, err := getSecurityGroupRuleSourceAccount(cs, sourceSG)
if err != nil {
errs = multierror.Append(errs, err)
continue
Expand All @@ -216,7 +230,8 @@ func createSecurityGroupRules(d *schema.ResourceData, meta interface{}, rules *s
}

p.SetSecuritygroupid(d.Id())
p.SetUsersecuritygrouplist(map[string]string{sg.Account: usg.(string)})
setSecurityGroupRuleOwnership(p, sg)
p.SetUsersecuritygrouplist(map[string]string{sourceAccount: usg.(string)})

// Create a single rule
err = createSecurityGroupRule(d, meta, rule, p, usg.(string))
Expand All @@ -240,6 +255,64 @@ func createSecurityGroupRules(d *schema.ResourceData, meta interface{}, rules *s
return errs.ErrorOrNil()
}

func setSecurityGroupRuleOwnership(p authorizeSecurityGroupParams, sg *cloudstack.SecurityGroup) {
if sg.Projectid != "" {
p.SetProjectid(sg.Projectid)
return
}

if sg.Domainid != "" {
p.SetDomainid(sg.Domainid)

if sg.Account != "" {
p.SetAccount(sg.Account)
}
}
}

func getSecurityGroupRuleSourceGroup(cs *cloudstack.CloudStackClient, targetSG *cloudstack.SecurityGroup, name string) (*cloudstack.SecurityGroup, error) {
p := cs.SecurityGroup.NewListSecurityGroupsParams()
p.SetSecuritygroupname(name)

if targetSG.Projectid != "" {
p.SetProjectid(targetSG.Projectid)
}

l, err := cs.SecurityGroup.ListSecurityGroups(p)
if err != nil {
return nil, err
}

if l.Count == 0 {
return nil, fmt.Errorf("No match found for security group %q", name)
}

if l.Count > 1 {
return nil, fmt.Errorf("There is more then one result for SecurityGroup name: %s", name)
}

return l.SecurityGroups[0], nil
}

func getSecurityGroupRuleSourceAccount(cs *cloudstack.CloudStackClient, sourceSG *cloudstack.SecurityGroup) (string, error) {
if sourceSG.Projectid == "" {
if sourceSG.Account == "" {
return "", fmt.Errorf("security group %q is missing account ownership", sourceSG.Name)
}
return sourceSG.Account, nil
}

project, _, err := cs.Project.GetProjectByID(sourceSG.Projectid)
if err != nil {
return "", err
}
if project.Projectaccountname == "" {
return "", fmt.Errorf("project %q is missing project account ownership", sourceSG.Projectid)
}

return project.Projectaccountname, nil
}

func createSecurityGroupRule(d *schema.ResourceData, meta interface{}, rule map[string]interface{}, p authorizeSecurityGroupParams, uuid string) error {
cs := meta.(*cloudstack.CloudStackClient)
uuids := rule["uuids"].(map[string]interface{})
Expand Down
150 changes: 147 additions & 3 deletions cloudstack/resource_cloudstack_security_group_rule_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -39,6 +39,7 @@ func TestAccCloudStackSecurityGroupRule_basic(t *testing.T) {
Config: testAccCloudStackSecurityGroupRule_basic,
Check: resource.ComposeTestCheckFunc(
testAccCheckCloudStackSecurityGroupRulesExist("cloudstack_security_group.foo"),
testAccCheckCloudStackSecurityGroupRuleTargetScope("cloudstack_security_group.foo", false),
resource.TestCheckResourceAttr(
"cloudstack_security_group_rule.foo", "rule.#", "3"),
resource.TestCheckTypeSetElemNestedAttrs(
Expand Down Expand Up @@ -69,6 +70,47 @@ func TestAccCloudStackSecurityGroupRule_basic(t *testing.T) {
})
}

func TestCloudStackSecurityGroupRuleOwnership_project(t *testing.T) {
p := new(cloudstack.SecurityGroupService).NewAuthorizeSecurityGroupIngressParams()
sg := &cloudstack.SecurityGroup{
Account: "admin",
Domainid: "domain-id",
Projectid: "project-id",
}

setSecurityGroupRuleOwnership(p, sg)

if projectID, ok := p.GetProjectid(); !ok || projectID != "project-id" {
t.Fatalf("expected project-id ownership, got %q, %t", projectID, ok)
}

if account, ok := p.GetAccount(); ok {
t.Fatalf("expected no account ownership for project rule, got %q", account)
}
}

func TestCloudStackSecurityGroupRuleOwnership_account(t *testing.T) {
p := new(cloudstack.SecurityGroupService).NewAuthorizeSecurityGroupEgressParams()
sg := &cloudstack.SecurityGroup{
Account: "admin",
Domainid: "domain-id",
}

setSecurityGroupRuleOwnership(p, sg)

if account, ok := p.GetAccount(); !ok || account != "admin" {
t.Fatalf("expected admin account ownership, got %q, %t", account, ok)
}

if domainID, ok := p.GetDomainid(); !ok || domainID != "domain-id" {
t.Fatalf("expected domain-id ownership, got %q, %t", domainID, ok)
}

if projectID, ok := p.GetProjectid(); ok {
t.Fatalf("expected no project ownership for account rule, got %q", projectID)
}
}

func TestAccCloudStackSecurityGroupRule_update(t *testing.T) {
resource.Test(t, resource.TestCase{
PreCheck: func() { testAccPreCheck(t) },
Expand All @@ -79,6 +121,7 @@ func TestAccCloudStackSecurityGroupRule_update(t *testing.T) {
Config: testAccCloudStackSecurityGroupRule_basic,
Check: resource.ComposeTestCheckFunc(
testAccCheckCloudStackSecurityGroupRulesExist("cloudstack_security_group.foo"),
testAccCheckCloudStackSecurityGroupRuleTargetScope("cloudstack_security_group.foo", false),
resource.TestCheckResourceAttr(
"cloudstack_security_group_rule.foo", "rule.#", "3"),
resource.TestCheckTypeSetElemNestedAttrs(
Expand Down Expand Up @@ -110,6 +153,7 @@ func TestAccCloudStackSecurityGroupRule_update(t *testing.T) {
Config: testAccCloudStackSecurityGroupRule_update,
Check: resource.ComposeTestCheckFunc(
testAccCheckCloudStackSecurityGroupRulesExist("cloudstack_security_group.foo"),
testAccCheckCloudStackSecurityGroupRuleTargetScope("cloudstack_security_group.foo", false),
resource.TestCheckResourceAttr(
"cloudstack_security_group_rule.foo", "rule.#", "4"),
resource.TestCheckTypeSetElemNestedAttrs(
Expand Down Expand Up @@ -147,6 +191,39 @@ func TestAccCloudStackSecurityGroupRule_update(t *testing.T) {
})
}

func TestAccCloudStackSecurityGroupRule_project(t *testing.T) {
resource.Test(t, resource.TestCase{
PreCheck: func() { testAccPreCheck(t) },
Providers: testAccProviders,
CheckDestroy: testAccCheckCloudStackSecurityGroupRuleDestroy,
Steps: []resource.TestStep{
{
Config: testAccCloudStackSecurityGroupRule_project,
Check: resource.ComposeTestCheckFunc(
testAccCheckCloudStackSecurityGroupRulesExist("cloudstack_security_group.foo"),
testAccCheckCloudStackSecurityGroupRuleTargetScope("cloudstack_security_group.foo", true),
resource.TestCheckResourceAttr(
"cloudstack_security_group.foo", "project", "terraform"),
resource.TestCheckResourceAttr(
"cloudstack_security_group_rule.foo", "project", "terraform"),
resource.TestCheckResourceAttr(
"cloudstack_security_group_rule.foo", "rule.#", "1"),
resource.TestCheckResourceAttr(
"cloudstack_security_group_rule.foo", "rule.0.protocol", "tcp"),
resource.TestCheckResourceAttr(
"cloudstack_security_group_rule.foo", "rule.0.ports.#", "1"),
resource.TestCheckResourceAttr(
"cloudstack_security_group_rule.foo", "rule.0.ports.0", "80"),
resource.TestCheckResourceAttr(
"cloudstack_security_group_rule.foo", "rule.0.traffic_type", "ingress"),
resource.TestCheckResourceAttr(
"cloudstack_security_group_rule.foo", "rule.0.user_security_group_list.0", "terraform-project-security-group-bar"),
),
},
},
})
}

func testAccCheckCloudStackSecurityGroupRulesExist(n string) resource.TestCheckFunc {
return func(s *terraform.State) error {
rs, ok := s.RootModule().Resources[n]
Expand All @@ -159,7 +236,10 @@ func testAccCheckCloudStackSecurityGroupRulesExist(n string) resource.TestCheckF
}

cs := testAccProvider.Meta().(*cloudstack.CloudStackClient)
sg, count, err := cs.SecurityGroup.GetSecurityGroupByID(rs.Primary.ID)
sg, count, err := cs.SecurityGroup.GetSecurityGroupByID(
rs.Primary.ID,
cloudstack.WithProject(rs.Primary.Attributes["project"]),
)
if err != nil {
if count == 0 {
return fmt.Errorf("Security group %s not found", rs.Primary.ID)
Expand Down Expand Up @@ -188,6 +268,40 @@ func testAccCheckCloudStackSecurityGroupRulesExist(n string) resource.TestCheckF
}
}

func testAccCheckCloudStackSecurityGroupRuleTargetScope(n string, wantProject bool) resource.TestCheckFunc {
return func(s *terraform.State) error {
rs, ok := s.RootModule().Resources[n]
if !ok {
return fmt.Errorf("Not found: %s", n)
}

cs := testAccProvider.Meta().(*cloudstack.CloudStackClient)
sg, _, err := cs.SecurityGroup.GetSecurityGroupByID(
rs.Primary.ID,
cloudstack.WithProject(rs.Primary.Attributes["project"]),
)
if err != nil {
return err
}

if wantProject {
if sg.Projectid == "" {
return fmt.Errorf("security group %s is not assigned to a project", rs.Primary.ID)
}
return nil
}

if sg.Projectid != "" {
return fmt.Errorf("security group %s is unexpectedly assigned to project %s", rs.Primary.ID, sg.Projectid)
}
if sg.Account == "" || sg.Domainid == "" {
return fmt.Errorf("security group %s is missing account/domain ownership", rs.Primary.ID)
}

return nil
}
}

func testAccCheckCloudStackSecurityGroupRuleDestroy(s *terraform.State) error {
cs := testAccProvider.Meta().(*cloudstack.CloudStackClient)

Expand All @@ -200,7 +314,10 @@ func testAccCheckCloudStackSecurityGroupRuleDestroy(s *terraform.State) error {
return fmt.Errorf("No security group rule ID is set")
}

sg, count, err := cs.SecurityGroup.GetSecurityGroupByID(rs.Primary.ID)
sg, count, err := cs.SecurityGroup.GetSecurityGroupByID(
rs.Primary.ID,
cloudstack.WithProject(rs.Primary.Attributes["project"]),
)
if err != nil {
if count == 0 {
continue
Expand Down Expand Up @@ -265,6 +382,33 @@ resource "cloudstack_security_group_rule" "foo" {
depends_on = ["cloudstack_security_group.bar"]
}`

const testAccCloudStackSecurityGroupRule_project = `
resource "cloudstack_security_group" "foo" {
name = "terraform-project-security-group-foo"
description = "terraform-security-group-text"
project = "terraform"
}

resource "cloudstack_security_group" "bar" {
name = "terraform-project-security-group-bar"
description = "terraform-security-group-text"
project = "terraform"
}

resource "cloudstack_security_group_rule" "foo" {
security_group_id = cloudstack_security_group.foo.id
project = "terraform"

rule {
protocol = "tcp"
ports = ["80"]
traffic_type = "ingress"
user_security_group_list = ["terraform-project-security-group-bar"]
}

depends_on = ["cloudstack_security_group.bar"]
}`

const testAccCloudStackSecurityGroupRule_update = `
resource "cloudstack_security_group" "foo" {
name = "terraform-security-group-foo"
Expand Down Expand Up @@ -306,5 +450,5 @@ resource "cloudstack_security_group_rule" "foo" {
user_security_group_list = ["terraform-security-group-bar"]
}

depends_on = ["cloudstack_security_group.bar"]
depends_on = ["cloudstack_security_group.bar"]
}`