From d66b1c125d84e8c01febfd48c440804179612796 Mon Sep 17 00:00:00 2001 From: Jian Qiu Date: Sat, 29 Jan 2022 00:51:17 +0800 Subject: [PATCH] fix: test secgroup rules to ovn ACL and recode guest adminSecgroup (#13358) Co-authored-by: Qiu Jian --- pkg/compute/models/guest_actions.go | 61 +++++++++-- pkg/compute/models/guests.go | 66 +++++++++-- pkg/compute/models/secgroups.go | 5 +- pkg/compute/options/options.go | 4 +- pkg/vpcagent/models/modelset.go | 11 +- pkg/vpcagent/models/secgrouprules_sort.go | 18 ++- pkg/vpcagent/ovn/secgrouprules_acl.go | 1 + pkg/vpcagent/ovn/secgrouprules_acl_test.go | 121 +++++++++++++++++++++ 8 files changed, 252 insertions(+), 35 deletions(-) create mode 100644 pkg/vpcagent/ovn/secgrouprules_acl_test.go diff --git a/pkg/compute/models/guest_actions.go b/pkg/compute/models/guest_actions.go index 3e5e0ece3d..c67c43ff95 100644 --- a/pkg/compute/models/guest_actions.go +++ b/pkg/compute/models/guest_actions.go @@ -1215,10 +1215,14 @@ func (self *SGuest) PerformAddSecgroup(ctx context.Context, userCred mcclient.To return nil, self.StartSyncTask(ctx, userCred, true, "") } -func (self *SGuest) saveDefaultSecgroupId(userCred mcclient.TokenCredential, secGrpId string) error { - if secGrpId != self.SecgrpId { +func (self *SGuest) saveDefaultSecgroupId(userCred mcclient.TokenCredential, secGrpId string, isAdmin bool) error { + if (!isAdmin && secGrpId != self.SecgrpId) || (isAdmin && secGrpId != self.AdminSecgrpId) { diff, err := db.Update(self, func() error { - self.SecgrpId = secGrpId + if isAdmin { + self.AdminSecgrpId = secGrpId + } else { + self.SecgrpId = secGrpId + } return nil }) if err != nil { @@ -1279,8 +1283,51 @@ func (self *SGuest) PerformRevokeSecgroup(ctx context.Context, userCred mcclient return nil, self.StartSyncTask(ctx, userCred, true, "") } +func (self *SGuest) PerformRevokeAdminSecgroup(ctx context.Context, userCred mcclient.TokenCredential, query jsonutils.JSONObject, input api.GuestRevokeSecgroupInput) (jsonutils.JSONObject, error) { + if !db.IsAdminAllowPerform(ctx, userCred, self, "revoke-admin-secgroup") { + return nil, httperrors.NewForbiddenError("not allow to revoke admin secgroup") + } + + if !utils.IsInStringArray(self.Status, []string{api.VM_READY, api.VM_RUNNING, api.VM_SUSPEND}) { + return nil, httperrors.NewInputParameterError("Cannot assign security rules in status %s", self.Status) + } + + var notes string + adminSecgrpId := "" + if len(options.Options.DefaultAdminSecurityGroupId) > 0 { + adminSecgrp, _ := SecurityGroupManager.FetchSecgroupById(options.Options.DefaultAdminSecurityGroupId) + if adminSecgrp != nil { + adminSecgrpId = adminSecgrp.Id + notes = fmt.Sprintf("reset admin secgroup to %s(%s)", adminSecgrp.Name, adminSecgrp.Id) + } + } + if adminSecgrpId == "" { + notes = "clean admin secgroup" + } + + err := self.saveDefaultSecgroupId(userCred, adminSecgrpId, true) + if err != nil { + return nil, errors.Wrap(err, "saveDefaultSecgroupId") + } + + logclient.AddActionLogWithContext(ctx, self, logclient.ACT_VM_REVOKESECGROUP, notes, userCred, true) + return nil, self.StartSyncTask(ctx, userCred, true, "") +} + // +onecloud:swagger-gen-ignore func (self *SGuest) PerformAssignSecgroup(ctx context.Context, userCred mcclient.TokenCredential, query jsonutils.JSONObject, input api.GuestAssignSecgroupInput) (jsonutils.JSONObject, error) { + return self.performAssignSecgroup(ctx, userCred, query, input, false) +} + +func (self *SGuest) PerformAssignAdminSecgroup(ctx context.Context, userCred mcclient.TokenCredential, query jsonutils.JSONObject, input api.GuestAssignSecgroupInput) (jsonutils.JSONObject, error) { + if !db.IsAdminAllowPerform(ctx, userCred, self, "assign-admin-secgroup") { + return nil, httperrors.NewForbiddenError("not allow to assign admin secgroup") + } + + return self.performAssignSecgroup(ctx, userCred, query, input, true) +} + +func (self *SGuest) performAssignSecgroup(ctx context.Context, userCred mcclient.TokenCredential, query jsonutils.JSONObject, input api.GuestAssignSecgroupInput, isAdmin bool) (jsonutils.JSONObject, error) { if !utils.IsInStringArray(self.Status, []string{api.VM_READY, api.VM_RUNNING, api.VM_SUSPEND}) { return nil, httperrors.NewInputParameterError("Cannot assign security rules in status %s", self.Status) } @@ -1299,12 +1346,12 @@ func (self *SGuest) PerformAssignSecgroup(ctx context.Context, userCred mcclient return nil, httperrors.NewInputParameterError("The secgroup name %s does not meet the requirements, please change the name", secObj.GetName()) } - err = self.saveDefaultSecgroupId(userCred, input.SecgroupId) + err = self.saveDefaultSecgroupId(userCred, input.SecgroupId, isAdmin) if err != nil { return nil, err } - notes := map[string]string{"name": secObj.GetName(), "id": secObj.GetId()} + notes := map[string]string{"name": secObj.GetName(), "id": secObj.GetId(), "is_admin": fmt.Sprintf("%v", isAdmin)} logclient.AddActionLogWithContext(ctx, self, logclient.ACT_VM_ASSIGNSECGROUP, notes, userCred, true) return nil, self.StartSyncTask(ctx, userCred, true, "") } @@ -1404,7 +1451,7 @@ func (self *SGuest) saveSecgroups(ctx context.Context, userCred mcclient.TokenCr return errors.Wrapf(err, "New guest secgroup for guest %s with secgroup %s", self.Name, id) } } - return self.saveDefaultSecgroupId(userCred, secgroupIds[0]) + return self.saveDefaultSecgroupId(userCred, secgroupIds[0], false) } func (self *SGuest) newGuestSecgroup(ctx context.Context, secgroupId string) error { @@ -2616,7 +2663,7 @@ func (self *SGuest) RevokeAllSecgroups(ctx context.Context, userCred mcclient.To return errors.Wrap(err, "Delete") } } - return self.saveDefaultSecgroupId(userCred, api.SECGROUP_DEFAULT_ID) + return self.saveDefaultSecgroupId(userCred, options.Options.DefaultSecurityGroupId, false) } func (self *SGuest) DoPendingDelete(ctx context.Context, userCred mcclient.TokenCredential) { diff --git a/pkg/compute/models/guests.go b/pkg/compute/models/guests.go index 526d03e591..f3f015afb5 100644 --- a/pkg/compute/models/guests.go +++ b/pkg/compute/models/guests.go @@ -143,7 +143,7 @@ type SGuest struct { // example: default SecgrpId string `width:"36" charset:"ascii" nullable:"true" list:"user" get:"user" create:"optional"` // 管理员可见安全组Id - AdminSecgrpId string `width:"36" charset:"ascii" nullable:"true" list:"admin" get:"domain"` + AdminSecgrpId string `width:"36" charset:"ascii" nullable:"true" list:"domain" get:"domain"` SrcIpCheck tristate.TriState `nullable:"false" default:"true" create:"optional" list:"user" update:"user"` SrcMacCheck tristate.TriState `nullable:"false" default:"true" create:"optional" list:"user" update:"user"` @@ -654,12 +654,13 @@ func (manager *SGuestManager) initHostname() error { return nil } -func (manager *SGuestManager) InitializeData() error { +func (manager *SGuestManager) clearSecgroups() error { guests := make([]SGuest, 0, 10) q := manager.Query() q = q.In("hypervisor", []string{api.HYPERVISOR_ESXI, api.HYPERVISOR_NUTANIX}).Filter( - sqlchemy.NOT( - sqlchemy.IsNullOrEmpty(q.Field("secgrp_id")), + sqlchemy.OR( + sqlchemy.IsNotEmpty(q.Field("secgrp_id")), + sqlchemy.IsNotEmpty(q.Field("admin_secgrp_id")), ), ) err := db.FetchModelObjects(manager, q, &guests) @@ -670,10 +671,50 @@ func (manager *SGuestManager) InitializeData() error { for i := range guests { db.Update(&guests[i], func() error { guests[i].SecgrpId = "" + guests[i].AdminSecgrpId = "" return nil }) } - return manager.initHostname() + return nil +} + +func (manager *SGuestManager) initAdminSecgroupId() error { + if len(options.Options.DefaultAdminSecurityGroupId) == 0 { + return nil + } + adminSec, _ := SecurityGroupManager.FetchSecgroupById(options.Options.DefaultAdminSecurityGroupId) + if adminSec == nil { + return nil + } + adminSecId := adminSec.Id + guests := make([]SGuest, 0, 10) + q := manager.Query() + q = q.In("hypervisor", []string{api.HYPERVISOR_KVM}).IsNullOrEmpty("admin_secgrp_id") + err := db.FetchModelObjects(manager, q, &guests) + if err != nil { + return errors.Wrap(err, "db.FetchModelObjects") + } + // remove secgroup for esxi nutanix guest + for i := range guests { + db.Update(&guests[i], func() error { + guests[i].AdminSecgrpId = adminSecId + return nil + }) + } + return nil +} + +func (manager *SGuestManager) InitializeData() error { + if err := manager.initHostname(); err != nil { + return errors.Wrap(err, "initHostname") + } + if err := manager.clearSecgroups(); err != nil { + return errors.Wrap(err, "cleanSecgroups") + } + if err := manager.initAdminSecgroupId(); err != nil { + return errors.Wrap(err, "initAdminSecgroupId") + } + return nil } func (guest *SGuest) GetHypervisor() string { @@ -907,6 +948,12 @@ func (guest *SGuest) IsNetworkAllocated() bool { } func (guest *SGuest) CustomizeCreate(ctx context.Context, userCred mcclient.TokenCredential, ownerId mcclient.IIdentityProvider, query jsonutils.JSONObject, data jsonutils.JSONObject) error { + if len(guest.SecgrpId) > 0 && len(options.Options.DefaultAdminSecurityGroupId) > 0 { + adminSec, _ := SecurityGroupManager.FetchSecgroupById(options.Options.DefaultAdminSecurityGroupId) + if adminSec != nil { + guest.AdminSecgrpId = adminSec.Id + } + } guest.HostId = "" return guest.SVirtualResourceBase.CustomizeCreate(ctx, userCred, ownerId, query, data) } @@ -1497,7 +1544,7 @@ func (manager *SGuestManager) validateCreateData( } input.SecgroupId = secGrpObj.GetId() } else { - input.SecgroupId = "default" + input.SecgroupId = options.Options.DefaultSecurityGroupId } maxSecgrpCount := GetDriver(hypervisor).GetMaxSecurityGroupCount() @@ -2383,8 +2430,8 @@ func (self *SGuest) getSecurityGroupsRules() string { q.Filter(sqlchemy.In(q.Field("secgroup_id"), secgroupids)).Desc(q.Field("priority"), q.Field("action")) secrules := []SSecurityGroupRule{} if err := db.FetchModelObjects(SecurityGroupRuleManager, q, &secrules); err != nil { - log.Errorf("Get rules error: %v", err) - return options.Options.DefaultSecurityRules + log.Errorf("Get security group rules error: %v", err) + return "" } rules := []string{} for _, rule := range secrules { @@ -2398,9 +2445,8 @@ func (self *SGuest) getAdminSecurityRules() string { if secgrp != nil { ret, _ := secgrp.getSecurityRuleString() return ret - } else { - return options.Options.DefaultAdminSecurityRules } + return "" } func (self *SGuest) isGpu() bool { diff --git a/pkg/compute/models/secgroups.go b/pkg/compute/models/secgroups.go index fda383871c..ade1f99879 100644 --- a/pkg/compute/models/secgroups.go +++ b/pkg/compute/models/secgroups.go @@ -1317,9 +1317,12 @@ func (sm *SSecurityGroupManager) TotalCnt(secIds []string) (map[string]api.SSecu } func (self *SSecurityGroup) ValidateDeleteCondition(ctx context.Context, info jsonutils.JSONObject) error { - if self.Id == api.SECGROUP_DEFAULT_ID { + if self.Id == options.Options.DefaultSecurityGroupId { return httperrors.NewProtectedResourceError("not allow to delete default security group") } + if self.Id == options.Options.DefaultAdminSecurityGroupId { + return httperrors.NewProtectedResourceError("not allow to delete default admin security group") + } cnts, err := SecurityGroupManager.TotalCnt([]string{self.Id}) if err != nil { return errors.Wrapf(err, "SecurityGroupManager.TotalCnt") diff --git a/pkg/compute/options/options.go b/pkg/compute/options/options.go index 29fc06ff79..d32d640085 100644 --- a/pkg/compute/options/options.go +++ b/pkg/compute/options/options.go @@ -31,8 +31,8 @@ type ComputeOptions struct { DefaultMemoryOvercommitBound float32 `default:"1.0" help:"Default memory overcommit bound for host, default to 1"` DefaultStorageOvercommitBound float32 `default:"1.0" help:"Default storage overcommit bound for storage, default to 1"` - DefaultSecurityRules string `help:"Default security rules" default:"allow any"` - DefaultAdminSecurityRules string `help:"Default admin security rules" default:""` + DefaultSecurityGroupId string `help:"Default security rules" default:"default"` + DefaultAdminSecurityGroupId string `help:"Default admin security rules" default:""` DefaultDiskSizeMB int `default:"10240" help:"Default disk size in MB if not specified, default to 10GiB" json:"default_disk_size"` diff --git a/pkg/vpcagent/models/modelset.go b/pkg/vpcagent/models/modelset.go index baa4ed117f..6a37015157 100644 --- a/pkg/vpcagent/models/modelset.go +++ b/pkg/vpcagent/models/modelset.go @@ -239,7 +239,7 @@ func (set Guests) joinHosts(subEntries Hosts) bool { func (set Guests) joinSecurityGroups(subEntries SecurityGroups) bool { correct := true - j := func(guest *Guest, fname, secgroupId string) (*SecurityGroup, bool) { + j := func(guest *Guest, fname, secgroupId string, isAdmin bool) (*SecurityGroup, bool) { if secgroupId == "" { return nil, true } @@ -249,12 +249,15 @@ func (set Guests) joinSecurityGroups(subEntries SecurityGroups) bool { fname, secgroupId, guest.Name, guest.Id) return nil, false } - guest.SecurityGroups[secgroupId] = secgroup + if !isAdmin { + // do not save admin security group, store it in AdminSecurityGroup instead + guest.SecurityGroups[secgroupId] = secgroup + } return secgroup, true } for _, g := range set { - adminSecgroup, c0 := j(g, "admin_secgrp_id", g.AdminSecgrpId) - _, c1 := j(g, "secgrp_id", g.SecgrpId) + adminSecgroup, c0 := j(g, "admin_secgrp_id", g.AdminSecgrpId, true) + _, c1 := j(g, "secgrp_id", g.SecgrpId, false) g.AdminSecurityGroup = adminSecgroup if !(c0 && c1) { correct = false diff --git a/pkg/vpcagent/models/secgrouprules_sort.go b/pkg/vpcagent/models/secgrouprules_sort.go index a879da3efc..1aa855199b 100644 --- a/pkg/vpcagent/models/secgrouprules_sort.go +++ b/pkg/vpcagent/models/secgrouprules_sort.go @@ -21,29 +21,25 @@ import ( func (el *Guest) OrderedSecurityGroupRules() []*SecurityGroupRule { rs := []*SecurityGroupRule{} for _, secgroup := range el.SecurityGroups { - rs = append(rs, secgroup.securityGroupRules()...) + rs = append(rs, secgroup.securityGroupRules(0)...) + } + if el.AdminSecurityGroup != nil { + rs = append(rs, el.AdminSecurityGroup.securityGroupRules(100)...) } sort.Slice(rs, SecurityGroupRuleLessFunc(rs)) - if el.AdminSecurityGroup != nil { - rs = append(rs, el.AdminSecurityGroup.OrderedSecurityGroupRules()...) - } return rs } -func (el *SecurityGroup) securityGroupRules() []*SecurityGroupRule { +func (el *SecurityGroup) securityGroupRules(basePriority int64) []*SecurityGroupRule { rs := make([]*SecurityGroupRule, 0, len(el.SecurityGroupRules)) for _, r := range el.SecurityGroupRules { + r = r.Copy() + r.Priority += basePriority rs = append(rs, r) } return rs } -func (el *SecurityGroup) OrderedSecurityGroupRules() []*SecurityGroupRule { - rs := el.securityGroupRules() - sort.Slice(rs, SecurityGroupRuleLessFunc(rs)) - return rs -} - func SecurityGroupRuleLessFunc(rs []*SecurityGroupRule) func(i, j int) bool { return func(i, j int) bool { return rs[i].Priority < rs[i].Priority diff --git a/pkg/vpcagent/ovn/secgrouprules_acl.go b/pkg/vpcagent/ovn/secgrouprules_acl.go index 56aab5f8e2..b40b0bfc6f 100644 --- a/pkg/vpcagent/ovn/secgrouprules_acl.go +++ b/pkg/vpcagent/ovn/secgrouprules_acl.go @@ -168,5 +168,6 @@ func ruleToAcl(lport string, rule *agentmodels.SecurityGroupRule) (*ovn_nb.ACL, Match: match, Action: action, } + return acl, nil } diff --git a/pkg/vpcagent/ovn/secgrouprules_acl_test.go b/pkg/vpcagent/ovn/secgrouprules_acl_test.go new file mode 100644 index 0000000000..54968a6962 --- /dev/null +++ b/pkg/vpcagent/ovn/secgrouprules_acl_test.go @@ -0,0 +1,121 @@ +// Copyright 2019 Yunion +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License + +package ovn + +import ( + "fmt" + "reflect" + "testing" + + "yunion.io/x/jsonutils" + "yunion.io/x/ovsdb/schema/ovn_nb" + "yunion.io/x/pkg/util/secrules" + + "yunion.io/x/onecloud/pkg/compute/models" + agentmodels "yunion.io/x/onecloud/pkg/vpcagent/models" +) + +func TestRuleToACL(t *testing.T) { + lport := "local-port120" + cases := []struct { + rule *agentmodels.SecurityGroupRule + acl *ovn_nb.ACL + }{ + { + // egress deny 100.10.10.0/24 + rule: &agentmodels.SecurityGroupRule{ + SSecurityGroupRule: models.SSecurityGroupRule{ + Direction: string(secrules.SecurityRuleEgress), + CIDR: "100.10.10.0/24", + Action: string(secrules.SecurityRuleDeny), + Protocol: secrules.PROTO_ANY, + Priority: 100, + }, + }, + acl: &ovn_nb.ACL{ + Direction: aclDirFromLport, + Action: "drop", + Match: fmt.Sprintf("inport == %q && ip4 && ip4.dst == 100.10.10.0/24", lport), + Priority: 100, + }, + }, + { + // egress allow any + rule: &agentmodels.SecurityGroupRule{ + SSecurityGroupRule: models.SSecurityGroupRule{ + Direction: string(secrules.SecurityRuleEgress), + CIDR: "", + Action: string(secrules.SecurityRuleAllow), + Protocol: secrules.PROTO_ANY, + Priority: 10, + }, + }, + acl: &ovn_nb.ACL{ + Direction: aclDirFromLport, + Action: "allow-related", + Match: fmt.Sprintf("inport == %q && ip4", lport), + Priority: 10, + }, + }, + { + // ingress deny all + rule: &agentmodels.SecurityGroupRule{ + SSecurityGroupRule: models.SSecurityGroupRule{ + Direction: string(secrules.SecurityRuleIngress), + CIDR: "", + Action: string(secrules.SecurityRuleDeny), + Protocol: secrules.PROTO_ANY, + Priority: 100, + }, + }, + acl: &ovn_nb.ACL{ + Direction: aclDirToLport, + Action: "drop", + Match: fmt.Sprintf("outport == %q && ip4", lport), + Priority: 100, + }, + }, + { + // ingress allow ssh + rule: &agentmodels.SecurityGroupRule{ + SSecurityGroupRule: models.SSecurityGroupRule{ + Direction: string(secrules.SecurityRuleIngress), + CIDR: "", + Action: string(secrules.SecurityRuleAllow), + Protocol: secrules.PROTO_TCP, + Ports: "22", + Priority: 100, + }, + }, + acl: &ovn_nb.ACL{ + Direction: aclDirToLport, + Action: "allow-related", + Match: fmt.Sprintf("outport == %q && ip4 && tcp && tcp.dst == 22", lport), + Priority: 100, + }, + }, + } + + for _, c := range cases { + got, err := ruleToAcl(lport, c.rule) + if err != nil { + t.Errorf("ruleToACL fail %s", err) + } else { + if !reflect.DeepEqual(got, c.acl) { + t.Errorf("want: %s got: %s", jsonutils.Marshal(c.acl), jsonutils.Marshal(got)) + } + } + } +}