From 5b370e56d0773f2f7a273d35f200cb8e2fb0ad1d Mon Sep 17 00:00:00 2001 From: Nawaz Hussain K Date: Wed, 23 Aug 2023 14:29:29 -0700 Subject: [PATCH] Users can add "Allow" or "Deny" action to the security rules - Users can add "Allow" or "Deny" action to the security rules of the Azure Network Security Group (NSG) that is associated with the Azure Cluster. --- api/v1beta1/azurecluster_default_test.go | 94 ++++++++++++++++ api/v1beta1/types.go | 16 +++ azure/converters/rules.go | 2 +- azure/scope/cluster.go | 2 + .../securitygroups/securitygroups_test.go | 5 + azure/services/securitygroups/spec_test.go | 104 ++++++++++++++++++ ...ucture.cluster.x-k8s.io_azureclusters.yaml | 18 +++ ...luster.x-k8s.io_azureclustertemplates.yaml | 20 ++++ docs/book/src/topics/custom-vnet.md | 17 +-- test/e2e/azure_securitygroups.go | 2 + 10 files changed, 272 insertions(+), 8 deletions(-) diff --git a/api/v1beta1/azurecluster_default_test.go b/api/v1beta1/azurecluster_default_test.go index eacbdb8fd96..a2fb87f0c90 100644 --- a/api/v1beta1/azurecluster_default_test.go +++ b/api/v1beta1/azurecluster_default_test.go @@ -696,6 +696,7 @@ func TestSubnetDefaults(t *testing.T) { DestinationPorts: ptr.To("*"), Source: ptr.To("*"), Destination: ptr.To("*"), + Action: SecurityRuleActionAllow, }, }, }, @@ -732,6 +733,99 @@ func TestSubnetDefaults(t *testing.T) { Source: ptr.To("*"), Destination: ptr.To("*"), Direction: SecurityRuleDirectionInbound, + Action: SecurityRuleActionAllow, + }, + }, + }, + Name: "my-custom-sg", + }, + }, + { + SubnetClassSpec: SubnetClassSpec{ + Role: SubnetNode, + CIDRBlocks: []string{DefaultNodeSubnetCIDR}, + Name: "cluster-test-node-subnet", + }, + SecurityGroup: SecurityGroup{Name: "cluster-test-node-nsg"}, + RouteTable: RouteTable{Name: "cluster-test-node-routetable"}, + NatGateway: NatGateway{ + NatGatewayIP: PublicIPSpec{ + Name: "", + }, + NatGatewayClassSpec: NatGatewayClassSpec{ + Name: "cluster-test-node-natgw", + }, + }, + }, + }, + }, + }, + }, + }, + { + name: "subnets with custom security group to deny port 49999", + cluster: &AzureCluster{ + ObjectMeta: metav1.ObjectMeta{ + Name: "cluster-test", + }, + Spec: AzureClusterSpec{ + NetworkSpec: NetworkSpec{ + Subnets: Subnets{ + { + SubnetClassSpec: SubnetClassSpec{ + Role: "control-plane", + Name: "cluster-test-controlplane-subnet", + }, + SecurityGroup: SecurityGroup{ + SecurityGroupClass: SecurityGroupClass{ + SecurityRules: []SecurityRule{ + { + Name: "deny_port_49999", + Description: "deny port 49999", + Protocol: "*", + Priority: 2201, + SourcePorts: ptr.To("*"), + DestinationPorts: ptr.To("*"), + Source: ptr.To("*"), + Destination: ptr.To("*"), + Action: SecurityRuleActionDeny, + }, + }, + }, + Name: "my-custom-sg", + }, + }, + }, + }, + }, + }, + output: &AzureCluster{ + ObjectMeta: metav1.ObjectMeta{ + Name: "cluster-test", + }, + Spec: AzureClusterSpec{ + NetworkSpec: NetworkSpec{ + Subnets: Subnets{ + { + SubnetClassSpec: SubnetClassSpec{ + Role: "control-plane", + CIDRBlocks: []string{DefaultControlPlaneSubnetCIDR}, + Name: "cluster-test-controlplane-subnet", + }, + SecurityGroup: SecurityGroup{ + SecurityGroupClass: SecurityGroupClass{ + SecurityRules: []SecurityRule{ + { + Name: "deny_port_49999", + Description: "deny port 49999", + Protocol: "*", + Priority: 2201, + SourcePorts: ptr.To("*"), + DestinationPorts: ptr.To("*"), + Source: ptr.To("*"), + Destination: ptr.To("*"), + Direction: SecurityRuleDirectionInbound, + Action: SecurityRuleActionDeny, }, }, }, diff --git a/api/v1beta1/types.go b/api/v1beta1/types.go index 62f6373b485..56a81c2066f 100644 --- a/api/v1beta1/types.go +++ b/api/v1beta1/types.go @@ -270,6 +270,17 @@ const ( SecurityRuleDirectionOutbound = SecurityRuleDirection("Outbound") ) +// SecurityRuleAccess defines the action type for a security group rule. +type SecurityRuleAccess string + +const ( + // SecurityRuleActionAllow allows traffic defined in the rule. + SecurityRuleActionAllow SecurityRuleAccess = "Allow" + + // SecurityRuleActionDeny denies traffic defined in the rule. + SecurityRuleActionDeny SecurityRuleAccess = "Deny" +) + // SecurityRule defines an Azure security rule for security groups. type SecurityRule struct { // Name is a unique name within the network security group. @@ -297,6 +308,11 @@ type SecurityRule struct { // Destination is the destination address prefix. CIDR or destination IP range. Asterix '*' can also be used to match all source IPs. Default tags such as 'VirtualNetwork', 'AzureLoadBalancer' and 'Internet' can also be used. // +optional Destination *string `json:"destination,omitempty"` + // Action specifies whether network traffic is allowed or denied. Can either be "Allow" or "Deny". Defaults to "Allow". + // +kubebuilder:default=Allow + // +kubebuilder:validation:Enum=Allow;Deny + //+optional + Action SecurityRuleAccess `json:"action"` } // SecurityRules is a slice of Azure security rules for security groups. diff --git a/azure/converters/rules.go b/azure/converters/rules.go index 3d9c5c75840..9ec95729a46 100644 --- a/azure/converters/rules.go +++ b/azure/converters/rules.go @@ -32,7 +32,7 @@ func SecurityRuleToSDK(rule infrav1.SecurityRule) network.SecurityRule { SourcePortRange: rule.SourcePorts, DestinationAddressPrefix: rule.Destination, DestinationPortRange: rule.DestinationPorts, - Access: network.SecurityRuleAccessAllow, + Access: network.SecurityRuleAccess(rule.Action), Priority: ptr.To[int32](rule.Priority), }, } diff --git a/azure/scope/cluster.go b/azure/scope/cluster.go index 91e110ecb0d..f19eb958df5 100644 --- a/azure/scope/cluster.go +++ b/azure/scope/cluster.go @@ -932,6 +932,7 @@ func (s *ClusterScope) SetControlPlaneSecurityRules() { SourcePorts: ptr.To("*"), Destination: ptr.To("*"), DestinationPorts: ptr.To("22"), + Action: infrav1.SecurityRuleActionAllow, }, infrav1.SecurityRule{ Name: "allow_apiserver", @@ -943,6 +944,7 @@ func (s *ClusterScope) SetControlPlaneSecurityRules() { SourcePorts: ptr.To("*"), Destination: ptr.To("*"), DestinationPorts: ptr.To(strconv.Itoa(int(s.APIServerPort()))), + Action: infrav1.SecurityRuleActionAllow, }, } s.AzureCluster.Spec.NetworkSpec.UpdateControlPlaneSubnet(subnet) diff --git a/azure/services/securitygroups/securitygroups_test.go b/azure/services/securitygroups/securitygroups_test.go index 5e1e8f858c3..4f59549ef27 100644 --- a/azure/services/securitygroups/securitygroups_test.go +++ b/azure/services/securitygroups/securitygroups_test.go @@ -70,6 +70,7 @@ var ( SourcePorts: ptr.To("*"), Destination: ptr.To("*"), DestinationPorts: ptr.To("22"), + Action: infrav1.SecurityRuleActionAllow, } securityRule2 = infrav1.SecurityRule{ Name: "other_rule", @@ -81,6 +82,7 @@ var ( SourcePorts: ptr.To("*"), Destination: ptr.To("*"), DestinationPorts: ptr.To("80"), + Action: infrav1.SecurityRuleActionAllow, } errFake = errors.New("this is an error") notDoneError = azure.NewOperationNotDoneError(&infrav1.Future{}) @@ -296,6 +298,7 @@ var ( SourceAddressPrefix: ptr.To("*"), Priority: ptr.To[int32](100), Direction: network.SecurityRuleDirectionInbound, + Access: network.SecurityRuleAccessAllow, }, } ruleB = network.SecurityRule{ @@ -309,6 +312,7 @@ var ( SourceAddressPrefix: ptr.To("*"), Priority: ptr.To[int32](100), Direction: network.SecurityRuleDirectionOutbound, + Access: network.SecurityRuleAccessAllow, }, } ruleBModified = network.SecurityRule{ @@ -322,6 +326,7 @@ var ( SourceAddressPrefix: ptr.To("*"), Priority: ptr.To[int32](100), Direction: network.SecurityRuleDirectionOutbound, + Access: network.SecurityRuleAccessAllow, }, } ) diff --git a/azure/services/securitygroups/spec_test.go b/azure/services/securitygroups/spec_test.go index 40a777e5670..4b3161d5f52 100644 --- a/azure/services/securitygroups/spec_test.go +++ b/azure/services/securitygroups/spec_test.go @@ -38,6 +38,7 @@ var ( SourcePorts: ptr.To("*"), Destination: ptr.To("*"), DestinationPorts: ptr.To("22"), + Action: infrav1.SecurityRuleActionAllow, } otherRule = infrav1.SecurityRule{ Name: "other_rule", @@ -49,6 +50,7 @@ var ( SourcePorts: ptr.To("*"), Destination: ptr.To("*"), DestinationPorts: ptr.To("80"), + Action: infrav1.SecurityRuleActionAllow, } customRule = infrav1.SecurityRule{ Name: "custom_rule", @@ -60,6 +62,19 @@ var ( SourcePorts: ptr.To("*"), Destination: ptr.To("*"), DestinationPorts: ptr.To("80"), + Action: infrav1.SecurityRuleActionAllow, + } + denyRule = infrav1.SecurityRule{ + Name: "deny_rule", + Description: "Deny Rule", + Priority: 510, + Protocol: infrav1.SecurityGroupProtocolTCP, + Direction: infrav1.SecurityRuleDirectionOutbound, + Source: ptr.To("*"), + SourcePorts: ptr.To("*"), + Destination: ptr.To("*"), + DestinationPorts: ptr.To("80"), + Action: infrav1.SecurityRuleActionDeny, } ) @@ -138,6 +153,48 @@ func TestParameters(t *testing.T) { })) }, }, + { + name: "NSG already exists but missing a rule", + spec: &NSGSpec{ + Name: "test-nsg", + Location: "test-location", + SecurityRules: infrav1.SecurityRules{ + sshRule, + otherRule, + }, + ResourceGroup: "test-group", + ClusterName: "my-cluster", + }, + existing: network.SecurityGroup{ + Name: ptr.To("test-nsg"), + Location: ptr.To("test-location"), + Etag: ptr.To("fake-etag"), + SecurityGroupPropertiesFormat: &network.SecurityGroupPropertiesFormat{ + SecurityRules: &[]network.SecurityRule{ + converters.SecurityRuleToSDK(sshRule), + converters.SecurityRuleToSDK(denyRule), + }, + }, + }, + expect: func(g *WithT, result interface{}) { + g.Expect(result).To(BeAssignableToTypeOf(network.SecurityGroup{})) + g.Expect(result).To(Equal(network.SecurityGroup{ + Location: ptr.To("test-location"), + Etag: ptr.To("fake-etag"), + SecurityGroupPropertiesFormat: &network.SecurityGroupPropertiesFormat{ + SecurityRules: &[]network.SecurityRule{ + converters.SecurityRuleToSDK(otherRule), + converters.SecurityRuleToSDK(sshRule), + converters.SecurityRuleToSDK(denyRule), + }, + }, + Tags: map[string]*string{ + "sigs.k8s.io_cluster-api-provider-azure_cluster_my-cluster": ptr.To("owned"), + "Name": ptr.To("test-nsg"), + }, + })) + }, + }, { name: "NSG already exists and a rule is deleted", spec: &NSGSpec{ @@ -185,6 +242,53 @@ func TestParameters(t *testing.T) { })) }, }, + { + name: "NSG already exists and a deny rule is deleted", + spec: &NSGSpec{ + Name: "test-nsg", + Location: "test-location", + SecurityRules: infrav1.SecurityRules{ + sshRule, + customRule, + }, + ResourceGroup: "test-group", + ClusterName: "my-cluster", + LastAppliedSecurityRules: map[string]interface{}{ + "allow_ssh": sshRule, + "custom_rule": customRule, + "deny_rule": denyRule, + }, + }, + existing: network.SecurityGroup{ + Name: ptr.To("test-nsg"), + Location: ptr.To("test-location"), + Etag: ptr.To("fake-etag"), + SecurityGroupPropertiesFormat: &network.SecurityGroupPropertiesFormat{ + SecurityRules: &[]network.SecurityRule{ + converters.SecurityRuleToSDK(sshRule), + converters.SecurityRuleToSDK(customRule), + converters.SecurityRuleToSDK(denyRule), + }, + }, + }, + expect: func(g *WithT, result interface{}) { + g.Expect(result).To(BeAssignableToTypeOf(network.SecurityGroup{})) + g.Expect(result).To(Equal(network.SecurityGroup{ + Location: ptr.To("test-location"), + Etag: ptr.To("fake-etag"), + SecurityGroupPropertiesFormat: &network.SecurityGroupPropertiesFormat{ + SecurityRules: &[]network.SecurityRule{ + converters.SecurityRuleToSDK(sshRule), + converters.SecurityRuleToSDK(customRule), + }, + }, + Tags: map[string]*string{ + "sigs.k8s.io_cluster-api-provider-azure_cluster_my-cluster": ptr.To("owned"), + "Name": ptr.To("test-nsg"), + }, + })) + }, + }, { name: "NSG already exists and a rule not owned by CAPZ is present", spec: &NSGSpec{ diff --git a/config/crd/bases/infrastructure.cluster.x-k8s.io_azureclusters.yaml b/config/crd/bases/infrastructure.cluster.x-k8s.io_azureclusters.yaml index 2bd43f2772d..83967b1d889 100644 --- a/config/crd/bases/infrastructure.cluster.x-k8s.io_azureclusters.yaml +++ b/config/crd/bases/infrastructure.cluster.x-k8s.io_azureclusters.yaml @@ -317,6 +317,15 @@ spec: description: SecurityRule defines an Azure security rule for security groups. properties: + action: + default: Allow + description: Action specifies whether network + traffic is allowed or denied. Can either be + "Allow" or "Deny". Defaults to "Allow". + enum: + - Allow + - Deny + type: string description: description: A description for this rule. Restricted to 140 chars. @@ -1022,6 +1031,15 @@ spec: description: SecurityRule defines an Azure security rule for security groups. properties: + action: + default: Allow + description: Action specifies whether network + traffic is allowed or denied. Can either be + "Allow" or "Deny". Defaults to "Allow". + enum: + - Allow + - Deny + type: string description: description: A description for this rule. Restricted to 140 chars. diff --git a/config/crd/bases/infrastructure.cluster.x-k8s.io_azureclustertemplates.yaml b/config/crd/bases/infrastructure.cluster.x-k8s.io_azureclustertemplates.yaml index 6508478b743..33cd31b0eea 100644 --- a/config/crd/bases/infrastructure.cluster.x-k8s.io_azureclustertemplates.yaml +++ b/config/crd/bases/infrastructure.cluster.x-k8s.io_azureclustertemplates.yaml @@ -200,6 +200,16 @@ spec: description: SecurityRule defines an Azure security rule for security groups. properties: + action: + default: Allow + description: Action specifies whether + network traffic is allowed or denied. + Can either be "Allow" or "Deny". Defaults + to "Allow". + enum: + - Allow + - Deny + type: string description: description: A description for this rule. Restricted to 140 chars. @@ -658,6 +668,16 @@ spec: description: SecurityRule defines an Azure security rule for security groups. properties: + action: + default: Allow + description: Action specifies whether + network traffic is allowed or denied. + Can either be "Allow" or "Deny". Defaults + to "Allow". + enum: + - Allow + - Deny + type: string description: description: A description for this rule. Restricted to 140 chars. diff --git a/docs/book/src/topics/custom-vnet.md b/docs/book/src/topics/custom-vnet.md index fe9949117d5..b3401fadb23 100644 --- a/docs/book/src/topics/custom-vnet.md +++ b/docs/book/src/topics/custom-vnet.md @@ -149,6 +149,7 @@ spec: destinationPorts: "22" source: "*" sourcePorts: "*" + action: "Allow" - name: "allow_apiserver" description: "Allow K8s API Server" direction: "Inbound" @@ -158,6 +159,7 @@ spec: destinationPorts: "6443" source: "*" sourcePorts: "*" + action: "Allow" - name: "allow_port_50000" description: "allow port 50000" direction: "Outbound" @@ -167,6 +169,7 @@ spec: destinationPorts: "50000" source: "*" sourcePorts: "*" + action: "Allow" - name: my-subnet-node role: node cidrBlocks: @@ -213,10 +216,10 @@ spec: ### Private Endpoints -A [Private Endpoint](https://learn.microsoft.com/en-us/azure/private-link/private-endpoint-overview) is a network interface that uses -a private IP address from your virtual network. This network interface connects you privately and securely to a service that's powered -by [Azure Private Link](https://learn.microsoft.com/en-us/azure/private-link/private-link-overview). Azure Private Link enables you -to access Azure PaaS Services (for example, Azure Storage and SQL Database) and Azure hosted customer-owned/partner services over a +A [Private Endpoint](https://learn.microsoft.com/en-us/azure/private-link/private-endpoint-overview) is a network interface that uses +a private IP address from your virtual network. This network interface connects you privately and securely to a service that's powered +by [Azure Private Link](https://learn.microsoft.com/en-us/azure/private-link/private-link-overview). Azure Private Link enables you +to access Azure PaaS Services (for example, Azure Storage and SQL Database) and Azure hosted customer-owned/partner services over a private endpoint in your virtual network. Private Endpoints are configured on a per-subnet basis. Vnets managed by either @@ -279,10 +282,10 @@ spec: privateEndpoints: - name: my-pe customNetworkInterfaceName: nic-my-pe # optional - applicationSecurityGroups: # optional + applicationSecurityGroups: # optional - - privateIPAddresses: # optional - - 10.0.2.10 + privateIPAddresses: # optional + - 10.0.2.10 location: eastus2 # optional privateLinkServiceConnections: - name: my-pls # optional diff --git a/test/e2e/azure_securitygroups.go b/test/e2e/azure_securitygroups.go index d2edccc799f..6ada8cce1e3 100644 --- a/test/e2e/azure_securitygroups.go +++ b/test/e2e/azure_securitygroups.go @@ -57,6 +57,7 @@ func AzureSecurityGroupsSpec(ctx context.Context, inputGetter func() AzureSecuri DestinationPorts: ptr.To("80"), Source: ptr.To("*"), Destination: ptr.To("*"), + Action: "Allow", } testSecurityRule2 = infrav1.SecurityRule{ Name: "test-security-rule-2", @@ -68,6 +69,7 @@ func AzureSecurityGroupsSpec(ctx context.Context, inputGetter func() AzureSecuri DestinationPorts: ptr.To("80"), Source: ptr.To("*"), Destination: ptr.To("*"), + Action: "Allow", } input AzureSecurityGroupsSpecInput )