diff --git a/pkg/monitor/azure/nsg/nsg.go b/pkg/monitor/azure/nsg/nsg.go new file mode 100644 index 00000000000..0996711a491 --- /dev/null +++ b/pkg/monitor/azure/nsg/nsg.go @@ -0,0 +1,176 @@ +package nsg + +// Copyright (c) Microsoft Corporation. +// Licensed under the Apache License 2.0. + +import ( + "context" + "net/http" + "net/netip" + "strings" + + "github.com/Azure/azure-sdk-for-go/sdk/azcore/arm" + mgmtnetwork "github.com/Azure/azure-sdk-for-go/services/network/mgmt/2020-08-01/network" + "github.com/Azure/go-autorest/autorest" + "github.com/sirupsen/logrus" + + "github.com/Azure/ARO-RP/pkg/api" + "github.com/Azure/ARO-RP/pkg/metrics" + "github.com/Azure/ARO-RP/pkg/util/azureclient" + "github.com/Azure/ARO-RP/pkg/util/azureclient/mgmt/network" +) + +const ( + DimSubnet = "subnet" + DimVnet = "vNet" + DimResourceGroup = "resourcegroup" + DimSubscriptionID = "subscriptionid" + DimSecurityGroup = "networksecuritygroup" + DimNSGRuleName = "rulename" + DimNSGRuleSources = "sources" + DimNSGRuleDestinations = "destinations" + DimNSGRuleDirection = "direction" + DimNSGRulePriority = "priority" + DimClusterResourceID = "clusterresourceid" + DimLocation = "location" + DimTenantID = "tenantid" + + MetricInvalidDenyRule = "preconfigurednsg.invalid.denyrule" + MetricSubnetAcessResponseCode = "preconfiguredNSG.subnetaccess.responsecode" + MetricSubnetAccessForbidden = "preconfigurednsg.subnetaccess.forbidden" + MetricUnsuccessfulFPCreation = "preconfigurednsg.fpcreation.unsuccessful" + MetricNSGMonitoringTimedOut = "preconfigurednsg.monitoring.timedout" + + expandNSG = "NetworkSecurityGroup" +) + +// NSGMonitor is responsible for performing NSG rule validations when preconfiguredNSG is enabled +type NSGMonitor struct { + log *logrus.Entry + emitter metrics.Emitter + oc *api.OpenShiftCluster + + subnetCli network.SubnetsClient + done chan error +} + +func (n *NSGMonitor) Done() <-chan error { + return n.done +} + +func NewNSGMonitor(log *logrus.Entry, oc *api.OpenShiftCluster, subscriptionID string, environment *azureclient.AROEnvironment, cred autorest.Authorizer, emitter metrics.Emitter) *NSGMonitor { + return &NSGMonitor{ + log: log, + emitter: emitter, + oc: oc, + + subnetCli: network.NewSubnetsClient(environment, subscriptionID, cred), + done: make(chan error), + } +} + +type subnetNSGConfig struct { + // subnet CIDR range + prefix netip.Prefix + // The rules from the subnet NSG + nsg *mgmtnetwork.SecurityGroup +} + +func (n *NSGMonitor) toSubnetConfig(ctx context.Context, subnetID string) (subnetNSGConfig, error) { + r, err := arm.ParseResourceID(subnetID) + if err != nil { + return subnetNSGConfig{}, err + } + + dims := map[string]string{ + DimClusterResourceID: n.oc.ID, + DimLocation: n.oc.Location, + DimSubnet: r.Name, + DimVnet: r.Parent.Name, + DimResourceGroup: r.ResourceGroupName, + DimSubscriptionID: r.SubscriptionID, + } + + subnet, err := n.subnetCli.Get(ctx, r.ResourceGroupName, r.Parent.Name, r.Name, expandNSG) + // response code is always returned as part of autorest + n.emitter.EmitGauge(MetricSubnetAcessResponseCode, int64(subnet.StatusCode), dims) + if err != nil { + if subnet.StatusCode == http.StatusForbidden { + n.emitter.EmitGauge(MetricSubnetAccessForbidden, int64(1), dims) + } + n.log.Errorf("Error while getting subnet %s. %s", subnetID, err) + return subnetNSGConfig{}, err + } + + cidr, err := toPrefix(*subnet.AddressPrefix) + if err != nil { + return subnetNSGConfig{}, err + } + return subnetNSGConfig{cidr, subnet.NetworkSecurityGroup}, nil +} + +func (n *NSGMonitor) Monitor(ctx context.Context) { + masterSubnet, err := n.toSubnetConfig(ctx, n.oc.Properties.MasterProfile.SubnetID) + if err != nil { + // FP has no access to the subnet + n.done <- err + return + } + + // need this to get the right workerProfiles + workerProfiles, _ := api.GetEnrichedWorkerProfiles(n.oc.Properties) + workerSubnets := make([]subnetNSGConfig, 0, len(workerProfiles)) + workerPrefixes := make([]netip.Prefix, 0, len(workerProfiles)) + for _, wp := range workerProfiles { + s, err := n.toSubnetConfig(ctx, wp.SubnetID) + if err != nil { + // FP has no access to the subnet + n.done <- err + return + } + workerSubnets = append(workerSubnets, s) + workerPrefixes = append(workerPrefixes, s.prefix) + } + + // to make sure each NSG is processed only once + nsgSet := map[string]*mgmtnetwork.SecurityGroup{ + *masterSubnet.nsg.ID: masterSubnet.nsg, + } + for _, w := range workerSubnets { + nsgSet[*w.nsg.ID] = w.nsg + } + + for nsgID, nsg := range nsgSet { + for _, rule := range *nsg.SecurityRules { + if rule.Access == mgmtnetwork.SecurityRuleAccessAllow { + // Allow rule - skip. + continue + } + // Deny rule + nsgResource, err := arm.ParseResourceID(nsgID) + if err != nil { + n.log.Errorf("Unable to parse NSG resource ID: %s. %s", nsgID, err) + continue + } + + r := newRuleChecker(n.log, masterSubnet.prefix, workerPrefixes, &rule) + + if r.isInvalidDenyRule() { + dims := map[string]string{ + DimClusterResourceID: n.oc.ID, + DimLocation: n.oc.Location, + DimSubscriptionID: nsgResource.SubscriptionID, + DimResourceGroup: nsgResource.ResourceGroupName, + DimSecurityGroup: nsgResource.Name, + DimNSGRuleName: *rule.Name, + DimNSGRuleSources: strings.Join(r.sourceStrings, ","), + DimNSGRuleDestinations: strings.Join(r.destinationStrings, ","), + DimNSGRuleDirection: string(rule.Direction), + DimNSGRulePriority: string(*rule.Priority), + } + n.emitter.EmitGauge(MetricInvalidDenyRule, int64(1), dims) + } + } + } + n.done <- nil +} diff --git a/pkg/monitor/azure/nsg/nsg_test.go b/pkg/monitor/azure/nsg/nsg_test.go new file mode 100644 index 00000000000..48e6c4578ca --- /dev/null +++ b/pkg/monitor/azure/nsg/nsg_test.go @@ -0,0 +1,464 @@ +package nsg + +// Copyright (c) Microsoft Corporation. +// Licensed under the Apache License 2.0. + +import ( + "context" + "errors" + "fmt" + "net/http" + "testing" + + "github.com/Azure/azure-sdk-for-go/services/network/mgmt/2020-08-01/network" + "github.com/Azure/go-autorest/autorest" + "github.com/golang/mock/gomock" + "github.com/sirupsen/logrus" + + "github.com/Azure/ARO-RP/pkg/api" + mock_network "github.com/Azure/ARO-RP/pkg/util/mocks/azureclient/mgmt/network" + mock_metrics "github.com/Azure/ARO-RP/pkg/util/mocks/metrics" + utilerror "github.com/Azure/ARO-RP/test/util/error" +) + +var ( + nsgRuleName1 = "RuleName1" + nsgRuleName2 = "RuleName2" + nsgRuleName3 = "RuleName3" + + priority1 int32 = 201 + priority2 int32 = 202 + priority3 int32 = 203 + + notOverlappingCIDR1 = "192.168.0.0/24" + notOverlappingCIDR2 = "172.28.0.0/24" + + subsetOfMaster1 = "10.0.0.2" + subsetOfMaster2 = "10.0.0.3/32" + + notOverlappingPrefixes1 = []string{"11.0.0.0/24", "11.0.1.0/24"} + notOverlappingPrefixes2 = []string{"12.0.0.0/24", "12.0.1.0/24"} + + overlappingWorkerPrefixes = []string{"10.0.1.1/32", "10.0.1.2"} + + subscriptionID = "0000-0000-0000-0000" + resourcegroupName = "myRG" + vNetName = "myVnet" + + masterSubnetName = "mastersubnet" + masterRange = "10.0.0.0/24" + masterSubnetID = fmt.Sprintf("/subscriptions/%s/resourceGroups/%s/providers/Microsoft.Network/virtualNetworks/%s/subnets/%s", subscriptionID, resourcegroupName, vNetName, masterSubnetName) + + workerSubnet1Name = "wsubnet1" + worker1Range = "10.0.1.0/24" + workerSubnet1ID = fmt.Sprintf("/subscriptions/%s/resourceGroups/%s/providers/Microsoft.Network/virtualNetworks/%s/subnets/%s", subscriptionID, resourcegroupName, vNetName, workerSubnet1Name) + + workerSubnet2Name = "wsubnet2" + worker2Range = "10.0.2.0/24" + workerSubnet2ID = fmt.Sprintf("/subscriptions/%s/resourceGroups/%s/providers/Microsoft.Network/virtualNetworks/%s/subnets/%s", subscriptionID, resourcegroupName, vNetName, workerSubnet2Name) + + masterSubnetMetricDimensions = map[string]string{ + DimClusterResourceID: ocID, + DimLocation: ocLocation, + DimSubnet: masterSubnetName, + DimVnet: vNetName, + DimResourceGroup: resourcegroupName, + DimSubscriptionID: subscriptionID, + } + + workerSubnet1MetricDimensions = map[string]string{ + DimClusterResourceID: ocID, + DimLocation: ocLocation, + DimSubnet: workerSubnet1Name, + DimVnet: vNetName, + DimResourceGroup: resourcegroupName, + DimSubscriptionID: subscriptionID, + } + + workerSubnet2MetricDimensions = map[string]string{ + DimClusterResourceID: ocID, + DimLocation: ocLocation, + DimSubnet: workerSubnet2Name, + DimVnet: vNetName, + DimResourceGroup: resourcegroupName, + DimSubscriptionID: subscriptionID, + } + + ocClusterName = "testing" + ocID = fmt.Sprintf("/subscriptions/%s/resourceGroups/%s/providers/Microsoft.RedHatOpenShift/OpenShiftClusters/%s", subscriptionID, resourcegroupName, ocClusterName) + ocLocation = "eastus" + oc = api.OpenShiftCluster{ + ID: ocID, + Location: ocLocation, + Properties: api.OpenShiftClusterProperties{ + MasterProfile: api.MasterProfile{ + SubnetID: masterSubnetID, + }, + WorkerProfiles: []api.WorkerProfile{ + { + SubnetID: workerSubnet1ID, + }, + { + SubnetID: workerSubnet2ID, + }, + }, + }, + } + + nsg1Name = "NSG-1" + nsg1ID = fmt.Sprintf("/subscriptions/%s/resourceGroups/%s/providers/Microsoft.Network/networkSecurityGroups/%s", subscriptionID, resourcegroupName, nsg1Name) + nsg2Name = "NSG-2" + nsg2ID = fmt.Sprintf("/subscriptions/%s/resourceGroups/%s/providers/Microsoft.Network/networkSecurityGroups/%s", subscriptionID, resourcegroupName, nsg2Name) +) + +func createBaseSubnets() (network.Subnet, network.Subnet, network.Subnet) { + subnets := make([]network.Subnet, 0, 3) + ranges := []string{masterRange, worker1Range, worker2Range} + + for i := 0; i < 3; i++ { + subnets = append( + subnets, + network.Subnet{ + Response: autorest.Response{ + Response: &http.Response{ + StatusCode: 200, + }, + }, + SubnetPropertiesFormat: &network.SubnetPropertiesFormat{ + AddressPrefix: &ranges[0], + }, + }, + ) + } + return subnets[0], subnets[1], subnets[2] +} + +func TestMonitor(t *testing.T) { + ctx := context.Background() + + for _, tt := range []struct { + name string + mockSubnet func(*mock_network.MockSubnetsClient) + mockEmitter func(*mock_metrics.MockEmitter) + wantErr string + }{ + { + name: "fail - forbidden access when retrieving worker subnet 2", + mockSubnet: func(mock *mock_network.MockSubnetsClient) { + masterSubnet, workerSubnet1, workerSubnet2 := createBaseSubnets() + workerSubnet2.StatusCode = 403 + + _1 := mock.EXPECT(). + Get(ctx, resourcegroupName, vNetName, masterSubnetName, expandNSG). + Return(masterSubnet, nil) + + _2 := mock.EXPECT(). + Get(ctx, resourcegroupName, vNetName, workerSubnet1Name, expandNSG). + Return(workerSubnet1, nil) + + _3 := mock.EXPECT(). + Get(ctx, resourcegroupName, vNetName, workerSubnet2Name, expandNSG). + Return(workerSubnet2, errors.New("Error while retrieving worker subnet 2")) + + gomock.InOrder(_1, _2, _3) + }, + mockEmitter: func(mock *mock_metrics.MockEmitter) { + _1 := mock.EXPECT().EmitGauge(MetricSubnetAcessResponseCode, int64(http.StatusOK), masterSubnetMetricDimensions) + _2 := mock.EXPECT().EmitGauge(MetricSubnetAcessResponseCode, int64(http.StatusOK), workerSubnet1MetricDimensions) + _3 := mock.EXPECT().EmitGauge(MetricSubnetAcessResponseCode, int64(http.StatusForbidden), workerSubnet2MetricDimensions) + _4 := mock.EXPECT().EmitGauge(MetricSubnetAccessForbidden, int64(1), workerSubnet2MetricDimensions) + + gomock.InOrder(_1, _2, _3, _4) + }, + wantErr: "Error while retrieving worker subnet 2", + }, + { + name: "pass - no Deny NSG rules", + mockSubnet: func(mock *mock_network.MockSubnetsClient) { + masterSubnet, workerSubnet1, workerSubnet2 := createBaseSubnets() + nsg := network.SecurityGroup{ + ID: &nsg1ID, + SecurityGroupPropertiesFormat: &network.SecurityGroupPropertiesFormat{ + SecurityRules: &[]network.SecurityRule{ + { + Name: &nsgRuleName1, + SecurityRulePropertiesFormat: &network.SecurityRulePropertiesFormat{ + Access: network.SecurityRuleAccessAllow, + }, + }, + { + Name: &nsgRuleName2, + SecurityRulePropertiesFormat: &network.SecurityRulePropertiesFormat{ + Access: network.SecurityRuleAccessAllow, + }, + }, + }, + }, + } + masterSubnet.NetworkSecurityGroup = &nsg + workerSubnet1.NetworkSecurityGroup = &nsg + workerSubnet2.NetworkSecurityGroup = &nsg + + _1 := mock.EXPECT(). + Get(ctx, resourcegroupName, vNetName, masterSubnetName, expandNSG). + Return(masterSubnet, nil) + + _2 := mock.EXPECT(). + Get(ctx, resourcegroupName, vNetName, workerSubnet1Name, expandNSG). + Return(workerSubnet1, nil) + + _3 := mock.EXPECT(). + Get(ctx, resourcegroupName, vNetName, workerSubnet2Name, expandNSG). + Return(workerSubnet2, nil) + + gomock.InOrder(_1, _2, _3) + }, + mockEmitter: func(mock *mock_metrics.MockEmitter) { + _1 := mock.EXPECT().EmitGauge(MetricSubnetAcessResponseCode, int64(http.StatusOK), masterSubnetMetricDimensions) + _2 := mock.EXPECT().EmitGauge(MetricSubnetAcessResponseCode, int64(http.StatusOK), workerSubnet1MetricDimensions) + _3 := mock.EXPECT().EmitGauge(MetricSubnetAcessResponseCode, int64(http.StatusOK), workerSubnet2MetricDimensions) + + gomock.InOrder(_1, _2, _3) + }, + }, + { + name: "pass - no rules", + mockSubnet: func(mock *mock_network.MockSubnetsClient) { + masterSubnet, workerSubnet1, workerSubnet2 := createBaseSubnets() + nsg := network.SecurityGroup{ + ID: &nsg1ID, + SecurityGroupPropertiesFormat: &network.SecurityGroupPropertiesFormat{ + SecurityRules: &[]network.SecurityRule{}, + }, + } + masterSubnet.NetworkSecurityGroup = &nsg + workerSubnet1.NetworkSecurityGroup = &nsg + workerSubnet2.NetworkSecurityGroup = &nsg + + _1 := mock.EXPECT(). + Get(ctx, resourcegroupName, vNetName, masterSubnetName, expandNSG). + Return(masterSubnet, nil) + + _2 := mock.EXPECT(). + Get(ctx, resourcegroupName, vNetName, workerSubnet1Name, expandNSG). + Return(workerSubnet1, nil) + + _3 := mock.EXPECT(). + Get(ctx, resourcegroupName, vNetName, workerSubnet2Name, expandNSG). + Return(workerSubnet2, nil) + + gomock.InOrder(_1, _2, _3) + }, + mockEmitter: func(mock *mock_metrics.MockEmitter) { + _1 := mock.EXPECT().EmitGauge(MetricSubnetAcessResponseCode, int64(http.StatusOK), masterSubnetMetricDimensions) + _2 := mock.EXPECT().EmitGauge(MetricSubnetAcessResponseCode, int64(http.StatusOK), workerSubnet1MetricDimensions) + _3 := mock.EXPECT().EmitGauge(MetricSubnetAcessResponseCode, int64(http.StatusOK), workerSubnet2MetricDimensions) + + gomock.InOrder(_1, _2, _3) + }, + }, + { + name: "pass - only irrelevant deny rules", + mockSubnet: func(mock *mock_network.MockSubnetsClient) { + masterSubnet, workerSubnet1, workerSubnet2 := createBaseSubnets() + masterSubnet.AddressPrefix = &masterRange + nsg1 := network.SecurityGroup{ + ID: &nsg1ID, + SecurityGroupPropertiesFormat: &network.SecurityGroupPropertiesFormat{ + SecurityRules: &[]network.SecurityRule{ + { + Name: &nsgRuleName1, + SecurityRulePropertiesFormat: &network.SecurityRulePropertiesFormat{ + Access: network.SecurityRuleAccessDeny, + SourceAddressPrefix: ¬OverlappingCIDR1, + DestinationAddressPrefix: ¬OverlappingCIDR2, + }, + }, + { + Name: &nsgRuleName1, + SecurityRulePropertiesFormat: &network.SecurityRulePropertiesFormat{ + Access: network.SecurityRuleAccessDeny, + SourceAddressPrefixes: ¬OverlappingPrefixes1, + DestinationAddressPrefixes: ¬OverlappingPrefixes2, + }, + }, + }, + }, + } + nsg2 := network.SecurityGroup{ + ID: &nsg2ID, + SecurityGroupPropertiesFormat: &network.SecurityGroupPropertiesFormat{ + SecurityRules: &[]network.SecurityRule{ + { + Name: &nsgRuleName1, + SecurityRulePropertiesFormat: &network.SecurityRulePropertiesFormat{ + Access: network.SecurityRuleAccessDeny, + SourceAddressPrefix: ¬OverlappingCIDR1, + DestinationAddressPrefix: ¬OverlappingCIDR2, + }, + }, + { + Name: &nsgRuleName1, + SecurityRulePropertiesFormat: &network.SecurityRulePropertiesFormat{ + Access: network.SecurityRuleAccessDeny, + SourceAddressPrefixes: ¬OverlappingPrefixes1, + DestinationAddressPrefixes: ¬OverlappingPrefixes2, + }, + }, + }, + }, + } + masterSubnet.NetworkSecurityGroup = &nsg1 + workerSubnet1.NetworkSecurityGroup = &nsg2 + workerSubnet2.NetworkSecurityGroup = &nsg2 + + _1 := mock.EXPECT(). + Get(ctx, resourcegroupName, vNetName, masterSubnetName, expandNSG). + Return(masterSubnet, nil) + + _2 := mock.EXPECT(). + Get(ctx, resourcegroupName, vNetName, workerSubnet1Name, expandNSG). + Return(workerSubnet1, nil) + + _3 := mock.EXPECT(). + Get(ctx, resourcegroupName, vNetName, workerSubnet2Name, expandNSG). + Return(workerSubnet2, nil) + + gomock.InOrder(_1, _2, _3) + }, + mockEmitter: func(mock *mock_metrics.MockEmitter) { + _1 := mock.EXPECT().EmitGauge(MetricSubnetAcessResponseCode, int64(http.StatusOK), masterSubnetMetricDimensions) + _2 := mock.EXPECT().EmitGauge(MetricSubnetAcessResponseCode, int64(http.StatusOK), workerSubnet1MetricDimensions) + _3 := mock.EXPECT().EmitGauge(MetricSubnetAcessResponseCode, int64(http.StatusOK), workerSubnet2MetricDimensions) + + gomock.InOrder(_1, _2, _3) + }, + }, + { + name: "pass - invalid deny rules, emitting metrics", + mockSubnet: func(mock *mock_network.MockSubnetsClient) { + masterSubnet, workerSubnet1, workerSubnet2 := createBaseSubnets() + masterSubnet.AddressPrefix = &masterRange + workerSubnet1.AddressPrefix = &worker1Range + workerSubnet2.AddressPrefix = &worker2Range + nsg1 := network.SecurityGroup{ + ID: &nsg1ID, + SecurityGroupPropertiesFormat: &network.SecurityGroupPropertiesFormat{ + SecurityRules: &[]network.SecurityRule{ + { + Name: &nsgRuleName1, + SecurityRulePropertiesFormat: &network.SecurityRulePropertiesFormat{ + Access: network.SecurityRuleAccessDeny, + SourceAddressPrefix: &subsetOfMaster1, + DestinationAddressPrefix: &subsetOfMaster2, + Priority: &priority1, + Direction: network.SecurityRuleDirectionInbound, + }, + }, + { + Name: &nsgRuleName2, + SecurityRulePropertiesFormat: &network.SecurityRulePropertiesFormat{ + Access: network.SecurityRuleAccessDeny, + SourceAddressPrefixes: ¬OverlappingPrefixes1, + DestinationAddressPrefixes: ¬OverlappingPrefixes2, + Priority: &priority2, + }, + }, + }, + }, + } + asterisk := "*" + nsg2 := network.SecurityGroup{ + ID: &nsg2ID, + SecurityGroupPropertiesFormat: &network.SecurityGroupPropertiesFormat{ + SecurityRules: &[]network.SecurityRule{ + { + Name: &nsgRuleName3, + SecurityRulePropertiesFormat: &network.SecurityRulePropertiesFormat{ + Access: network.SecurityRuleAccessDeny, + SourceAddressPrefixes: &overlappingWorkerPrefixes, + DestinationAddressPrefix: &asterisk, + Priority: &priority3, + Direction: network.SecurityRuleDirectionOutbound, + }, + }, + }, + }, + } + masterSubnet.NetworkSecurityGroup = &nsg1 + workerSubnet1.NetworkSecurityGroup = &nsg2 + workerSubnet2.NetworkSecurityGroup = &nsg2 + + _1 := mock.EXPECT(). + Get(ctx, resourcegroupName, vNetName, masterSubnetName, expandNSG). + Return(masterSubnet, nil) + + _2 := mock.EXPECT(). + Get(ctx, resourcegroupName, vNetName, workerSubnet1Name, expandNSG). + Return(workerSubnet1, nil) + + _3 := mock.EXPECT(). + Get(ctx, resourcegroupName, vNetName, workerSubnet2Name, expandNSG). + Return(workerSubnet2, nil) + + gomock.InOrder(_1, _2, _3) + }, + mockEmitter: func(mock *mock_metrics.MockEmitter) { + _1 := mock.EXPECT().EmitGauge(MetricSubnetAcessResponseCode, int64(http.StatusOK), masterSubnetMetricDimensions) + _2 := mock.EXPECT().EmitGauge(MetricSubnetAcessResponseCode, int64(http.StatusOK), workerSubnet1MetricDimensions) + _3 := mock.EXPECT().EmitGauge(MetricSubnetAcessResponseCode, int64(http.StatusOK), workerSubnet2MetricDimensions) + _4 := mock.EXPECT().EmitGauge(MetricInvalidDenyRule, int64(1), map[string]string{ + DimClusterResourceID: ocID, + DimLocation: ocLocation, + DimSubscriptionID: subscriptionID, + DimResourceGroup: resourcegroupName, + DimSecurityGroup: nsg1Name, + DimNSGRuleName: nsgRuleName1, + DimNSGRuleSources: subsetOfMaster1, + DimNSGRuleDestinations: subsetOfMaster2, + DimNSGRuleDirection: string(network.SecurityRuleDirectionInbound), + DimNSGRulePriority: string(priority1), + }) + _5 := mock.EXPECT().EmitGauge(MetricInvalidDenyRule, int64(1), map[string]string{ + DimClusterResourceID: ocID, + DimLocation: ocLocation, + DimSubscriptionID: subscriptionID, + DimResourceGroup: resourcegroupName, + DimSecurityGroup: nsg2Name, + DimNSGRuleName: nsgRuleName3, + DimNSGRuleSources: "10.0.1.1/32,10.0.1.2", + DimNSGRuleDestinations: "*", + DimNSGRuleDirection: string(network.SecurityRuleDirectionOutbound), + DimNSGRulePriority: string(priority3), + }) + gomock.InOrder(_1, _2, _3, _4, _5) + }, + }, + } { + t.Run(tt.name, func(t *testing.T) { + ctrl := gomock.NewController(t) + defer ctrl.Finish() + subnetClient := mock_network.NewMockSubnetsClient(ctrl) + emitter := mock_metrics.NewMockEmitter(ctrl) + + if tt.mockSubnet != nil { + tt.mockSubnet(subnetClient) + } + if tt.mockEmitter != nil { + tt.mockEmitter(emitter) + } + + n := NSGMonitor{ + log: logrus.NewEntry(logrus.New()), + oc: &oc, + subnetCli: subnetClient, + emitter: emitter, + done: make(chan error), + } + + go n.Monitor(ctx) + err := <-n.Done() + utilerror.AssertErrorMessage(t, err, tt.wantErr) + }) + } +} diff --git a/pkg/monitor/azure/nsg/rulechecker.go b/pkg/monitor/azure/nsg/rulechecker.go new file mode 100644 index 00000000000..b3a5cfa9b8b --- /dev/null +++ b/pkg/monitor/azure/nsg/rulechecker.go @@ -0,0 +1,182 @@ +package nsg + +// Copyright (c) Microsoft Corporation. +// Licensed under the Apache License 2.0. + +import ( + "fmt" + "net/netip" + + mgmtnetwork "github.com/Azure/azure-sdk-for-go/services/network/mgmt/2020-08-01/network" + "github.com/sirupsen/logrus" +) + +const ( + virtualNetwork = "VirtualNetwork" + wildcard = "*" +) + +type ruleChecker struct { + log *logrus.Entry + master netip.Prefix + workers []netip.Prefix + rule *mgmtnetwork.SecurityRule + sourceStrings, destinationStrings []string +} + +func newRuleChecker(log *logrus.Entry, master netip.Prefix, workers []netip.Prefix, rule *mgmtnetwork.SecurityRule) ruleChecker { + var sourceStrings, destinationStrings []string + if rule.SourceAddressPrefix != nil { + sourceStrings = append(sourceStrings, *rule.SourceAddressPrefix) + } + if rule.SourceAddressPrefixes != nil { + sourceStrings = append(sourceStrings, *rule.SourceAddressPrefixes...) + } + if rule.DestinationAddressPrefix != nil { + destinationStrings = append(destinationStrings, *rule.DestinationAddressPrefix) + } + if rule.DestinationAddressPrefixes != nil { + destinationStrings = append(destinationStrings, *rule.DestinationAddressPrefixes...) + } + return ruleChecker{ + log: log, + master: master, + workers: workers, + rule: rule, + sourceStrings: sourceStrings, + destinationStrings: destinationStrings, + } +} + +// For storing evaluation results +type properties struct { + isMaster bool + isWorker bool + isAny bool + isVirtualNetwork bool +} + +func (p properties) isNothing() bool { + return !p.isMaster && !p.isWorker && !p.isAny && !p.isVirtualNetwork +} + +func (c ruleChecker) newProperties(addresses []string) properties { + return properties{ + isMaster: c.isMaster(addresses), + isWorker: c.isWorker(addresses), + isAny: isAny(addresses), + isVirtualNetwork: isVirtualNetwork(addresses), + } +} + +func (c ruleChecker) isInvalidDenyRule() bool { + source := c.newProperties(c.sourceStrings) + destination := c.newProperties(c.destinationStrings) + + switch { + case source.isNothing() && destination.isNothing(): + return false + case source.isMaster && destination.isMaster: + return true + case source.isMaster && destination.isWorker: + return true + case source.isWorker && destination.isMaster: + return true + case source.isMaster && destination.isAny: + return true + case source.isAny && destination.isMaster: + return true + case source.isWorker && destination.isAny: + return true + case source.isAny && destination.isWorker: + return true + case source.isAny && destination.isAny: + return true + case source.isMaster && destination.isVirtualNetwork: + return true + case source.isVirtualNetwork && destination.isMaster: + return true + case source.isWorker && destination.isVirtualNetwork: + return true + case source.isVirtualNetwork && destination.isWorker: + return true + case source.isVirtualNetwork && destination.isAny: + return true + case source.isAny && destination.isVirtualNetwork: + return true + case source.isVirtualNetwork && destination.isVirtualNetwork: + return true + } + return false +} + +func (c ruleChecker) isMaster(addresses []string) bool { + prefixes := toPrefixes(c.log, addresses) + for _, p := range prefixes { + if c.master.Overlaps(p) { + return true + } + } + return false +} + +func (c ruleChecker) isWorker(addresses []string) bool { + prefixes := toPrefixes(c.log, addresses) + for _, wp := range c.workers { + for _, p := range prefixes { + if wp.Overlaps(p) { + return true + } + } + } + return false +} + +func isAny(addresses []string) bool { + for _, address := range addresses { + if address == wildcard { + return true + } + } + return false +} + +func isVirtualNetwork(addresses []string) bool { + for _, address := range addresses { + if address == virtualNetwork { + return true + } + } + return false +} + +// toPrefix converts a string value of an IP address or a CIDR range into a prefix +func toPrefix(value string) (netip.Prefix, error) { + if addr, err := netip.ParseAddr(value); err == nil { + return netip.PrefixFrom(addr, 32), nil + } + if prefix, err := netip.ParsePrefix(value); err == nil { + return prefix, nil + } + return netip.Prefix{}, fmt.Errorf("invalid IP address or CIDR range: %s", value) +} + +// toPrefixes converts a slice of addresses into a slice of Prefixes +func toPrefixes(log *logrus.Entry, addresses []string) []netip.Prefix { + prefixes := make([]netip.Prefix, 0, len(addresses)) + for _, address := range addresses { + if address == wildcard || address == virtualNetwork { + continue // skip + } + prefix, err := toPrefix(address) + if err != nil { + log.Errorf("Unable to parse %s to prefix. Full error: %s", address, err) + // This won't likely happen because Azure enforces the input format. + // But in the very unlikely event, we still don't want to stop here. + // Even if the value is wrong, it won't be neither master or worker. + continue + } + prefixes = append(prefixes, prefix) + } + return prefixes +} diff --git a/pkg/monitor/azure/nsg/rulechecker_test.go b/pkg/monitor/azure/nsg/rulechecker_test.go new file mode 100644 index 00000000000..307cccb464b --- /dev/null +++ b/pkg/monitor/azure/nsg/rulechecker_test.go @@ -0,0 +1,599 @@ +package nsg + +// Copyright (c) Microsoft Corporation. +// Licensed under the Apache License 2.0. + +import ( + "net/netip" + "testing" + + mgmtnetwork "github.com/Azure/azure-sdk-for-go/services/network/mgmt/2020-08-01/network" + "github.com/sirupsen/logrus" + + utilerror "github.com/Azure/ARO-RP/test/util/error" +) + +func TestToPrefix(t *testing.T) { + for _, tt := range []struct { + name string + in string + want netip.Prefix + wantErr string + }{ + { + name: "pass - valid CIDR range", + in: "10.0.0.0/24", + want: netip.MustParsePrefix("10.0.0.0/24"), + }, + { + name: "failed - invalid CIDR range", + in: "10.0.0.0/33", + want: netip.Prefix{}, + wantErr: `invalid IP address or CIDR range: 10.0.0.0/33`, + }, + { + name: "pass - valid IP address", + in: "10.0.0.1", + want: netip.PrefixFrom(netip.MustParseAddr("10.0.0.1"), 32), + }, + { + name: "failed - invalid IP address", + in: "300.0.0.1", + want: netip.Prefix{}, + wantErr: `invalid IP address or CIDR range: 300.0.0.1`, + }, + { + name: "failed - blank", + in: "", + want: netip.Prefix{}, + wantErr: `invalid IP address or CIDR range: `, + }, + } { + t.Run(tt.name, func(t *testing.T) { + got, err := toPrefix(tt.in) + utilerror.AssertErrorMessage(t, err, tt.wantErr) + if got != tt.want { + t.Errorf("getPrefix got %s want %s", got, tt.want) + } + }) + } +} + +func TestToPrefixes(t *testing.T) { + log := logrus.NewEntry(logrus.New()) + for _, tt := range []struct { + name string + in []string + want []netip.Prefix + }{ + { + name: "pass - valid IPs only", + in: []string{"10.0.0.1", "10.0.0.2", "10.0.0.3"}, + want: []netip.Prefix{ + netip.MustParsePrefix("10.0.0.1/32"), + netip.MustParsePrefix("10.0.0.2/32"), + netip.MustParsePrefix("10.0.0.3/32"), + }, + }, + { + name: "pass - valid CIDRs only", + in: []string{"10.0.0.0/24", "10.0.1.0/24", "10.0.2.0/24"}, + want: []netip.Prefix{ + netip.MustParsePrefix("10.0.0.0/24"), + netip.MustParsePrefix("10.0.1.0/24"), + netip.MustParsePrefix("10.0.2.0/24"), + }, + }, + { + name: "pass - combination of IPs and CIDRs", + in: []string{"10.0.0.0/24", "10.0.1.0", "10.0.2.0/24", "10.0.3.0"}, + want: []netip.Prefix{ + netip.MustParsePrefix("10.0.0.0/24"), + netip.MustParsePrefix("10.0.1.0/32"), + netip.MustParsePrefix("10.0.2.0/24"), + netip.MustParsePrefix("10.0.3.0/32"), + }, + }, + { + name: "pass - skip invalid IPs and CIDRs", + in: []string{"10.0.0.0/24", "1000.0.1.0", "1000.0.2.0/24", "10.0.3.0"}, + want: []netip.Prefix{ + netip.MustParsePrefix("10.0.0.0/24"), + netip.MustParsePrefix("10.0.3.0/32"), + }, + }, + { + name: "pass - blank", + in: []string{}, + want: []netip.Prefix{}, + }, + } { + t.Run(tt.name, func(t *testing.T) { + got := toPrefixes(log, tt.in) + if len(got) != len(tt.want) { + t.Errorf("want result length %d, got %d", len(tt.want), len(got)) + } + for i := range got { + if got[i] != tt.want[i] { + t.Errorf("want %s got %s on index %d", tt.want[i], got[i], i) + } + } + }) + } +} + +func TestIsVirtualNetwork(t *testing.T) { + for _, tt := range []struct { + name string + in []string + want bool + }{ + { + name: "true", + in: []string{"this is not", "VirtualNetwork"}, + want: true, + }, + { + name: "false - no VirtualNetwork", + in: []string{"this is not", "vNet"}, + want: false, + }, + { + name: "false - blank", + in: []string{}, + want: false, + }, + } { + t.Run(tt.name, func(t *testing.T) { + got := isVirtualNetwork(tt.in) + if got != tt.want { + t.Errorf("want %t, got %t", tt.want, got) + } + }) + } +} + +func TestIsAny(t *testing.T) { + for _, tt := range []struct { + name string + in []string + want bool + }{ + { + name: "true", + in: []string{"this is not", "*"}, + want: true, + }, + { + name: "false - no VirtualNetwork", + in: []string{"this is not", "vNet"}, + want: false, + }, + { + name: "false - blank", + in: []string{}, + want: false, + }, + } { + t.Run(tt.name, func(t *testing.T) { + got := isAny(tt.in) + if got != tt.want { + t.Errorf("want %t, got %t", tt.want, got) + } + }) + } +} + +func TestRuleCheckerIsMaster(t *testing.T) { + generator := func(c *ruleChecker) { + c.master = netip.MustParsePrefix("10.0.0.0/24") + } + for _, tt := range []struct { + name string + mod func(*ruleChecker) + in []string + want bool + }{ + { + name: "true - overlapping", + mod: generator, + in: []string{ + "10.0.0.1/32", + "192.168.0.0/24", + }, + want: true, + }, + { + name: "false - non-overlapping", + mod: generator, + in: []string{ + "172.28.0.1/32", + "192.168.0.0/24", + }, + want: false, + }, + { + name: "false - not overlapping with nothing", + mod: generator, + in: []string{}, + want: false, + }, + { + name: "false - uninitialized checker", + mod: func(c *ruleChecker) { + // set nothing + }, + in: []string{}, + want: false, + }, + } { + t.Run(tt.name, func(t *testing.T) { + c := ruleChecker{} + tt.mod(&c) + got := c.isMaster(tt.in) + if got != tt.want { + t.Errorf("want %t, got %t", tt.want, got) + } + }) + } +} + +func TestRuleCheckerIsWorker(t *testing.T) { + generator := func(c *ruleChecker) { + c.workers = []netip.Prefix{ + netip.MustParsePrefix("10.0.0.0/24"), + netip.MustParsePrefix("10.0.1.0/24"), + } + } + for _, tt := range []struct { + name string + mod func(*ruleChecker) + in []string + want bool + }{ + { + name: "true - overlapping", + mod: generator, + in: []string{ + "10.0.0.1/32", + "192.168.0.0/24", + }, + want: true, + }, + { + name: "false - non-overlapping", + mod: generator, + in: []string{ + "172.28.0.1/32", + "192.168.0.0/24", + }, + want: false, + }, + { + name: "false - not overlapping with nothing", + mod: generator, + in: []string{}, + want: false, + }, + { + name: "false - uninitialized checker", + mod: func(c *ruleChecker) { + // set nothing + }, + in: []string{}, + want: false, + }, + } { + t.Run(tt.name, func(t *testing.T) { + c := ruleChecker{} + tt.mod(&c) + got := c.isWorker(tt.in) + if got != tt.want { + t.Errorf("want %t, got %t", tt.want, got) + } + }) + } +} + +func TestPropertiesIsNothing(t *testing.T) { + for _, tt := range []struct { + name string + in properties + want bool + }{ + { + name: "nothing", + in: properties{false, false, false, false}, + want: true, + }, + { + name: "something - isMaster", + in: properties{true, false, false, false}, + want: false, + }, + { + name: "all", + in: properties{true, true, true, true}, + want: false, + }, + } { + t.Run(tt.name, func(t *testing.T) { + got := tt.in.isNothing() + if got != tt.want { + t.Errorf("want %t, got %t", tt.want, got) + } + }) + } +} + +func TestRuleCheckerIsInvalidDenyRule(t *testing.T) { + log := logrus.NewEntry(logrus.New()) + masterPrefix := netip.MustParsePrefix("10.0.0.0/24") + workerPrefixes := []netip.Prefix{ + netip.MustParsePrefix("10.0.1.0/24"), + netip.MustParsePrefix("10.0.2.0/24"), + } + var ( + nothing = "172.28.0.0/24" + master = "10.0.0.1/32" + masters = []string{"10.0.0.1/32", "10.0.0.2/32"} + worker = "10.0.1.1/32" + workers = []string{"10.0.1.1/32", "10.0.1.2/32"} + wildcard = "*" + vnet = "VirtualNetwork" + ) + for _, tt := range []struct { + name string + rule mgmtnetwork.SecurityRule + want bool + }{ + { + name: "valid - both nothing", + rule: mgmtnetwork.SecurityRule{ + SecurityRulePropertiesFormat: &mgmtnetwork.SecurityRulePropertiesFormat{ + SourceAddressPrefix: ¬hing, + DestinationAddressPrefix: ¬hing, + }, + }, + want: false, + }, + { + name: "valid - source master, destination nothing", + rule: mgmtnetwork.SecurityRule{ + SecurityRulePropertiesFormat: &mgmtnetwork.SecurityRulePropertiesFormat{ + SourceAddressPrefix: &master, + DestinationAddressPrefix: ¬hing, + }, + }, + want: false, + }, + { + name: "valid - source master (prefixes), destination nothing", + rule: mgmtnetwork.SecurityRule{ + SecurityRulePropertiesFormat: &mgmtnetwork.SecurityRulePropertiesFormat{ + SourceAddressPrefixes: &masters, + DestinationAddressPrefix: ¬hing, + }, + }, + want: false, + }, + { + name: "valid - source nothing, destination master", + rule: mgmtnetwork.SecurityRule{ + SecurityRulePropertiesFormat: &mgmtnetwork.SecurityRulePropertiesFormat{ + SourceAddressPrefix: ¬hing, + DestinationAddressPrefix: &master, + }, + }, + want: false, + }, + { + name: "invalid - source master, destination master", + rule: mgmtnetwork.SecurityRule{ + SecurityRulePropertiesFormat: &mgmtnetwork.SecurityRulePropertiesFormat{ + SourceAddressPrefix: &master, + DestinationAddressPrefix: &master, + }, + }, + want: true, + }, + { + name: "invalid - source master, destination worker", + rule: mgmtnetwork.SecurityRule{ + SecurityRulePropertiesFormat: &mgmtnetwork.SecurityRulePropertiesFormat{ + SourceAddressPrefix: &master, + DestinationAddressPrefix: &worker, + }, + }, + want: true, + }, + { + name: "invalid - source worker (prefixes), destination master", + rule: mgmtnetwork.SecurityRule{ + SecurityRulePropertiesFormat: &mgmtnetwork.SecurityRulePropertiesFormat{ + SourceAddressPrefixes: &workers, + DestinationAddressPrefix: &master, + }, + }, + want: true, + }, + { + name: "valid - source worker (prefixes), destination worker", + rule: mgmtnetwork.SecurityRule{ + SecurityRulePropertiesFormat: &mgmtnetwork.SecurityRulePropertiesFormat{ + SourceAddressPrefixes: &workers, + DestinationAddressPrefix: &worker, + }, + }, + want: false, + }, + { + name: "invalid - source master (prefixes), destination any", + rule: mgmtnetwork.SecurityRule{ + SecurityRulePropertiesFormat: &mgmtnetwork.SecurityRulePropertiesFormat{ + SourceAddressPrefixes: &masters, + DestinationAddressPrefix: &wildcard, + }, + }, + want: true, + }, + { + name: "invalid - source workers (prefixes), destination any", + rule: mgmtnetwork.SecurityRule{ + SecurityRulePropertiesFormat: &mgmtnetwork.SecurityRulePropertiesFormat{ + SourceAddressPrefixes: &workers, + DestinationAddressPrefix: &wildcard, + }, + }, + want: true, + }, + { + name: "invalid - source any, destination master", + rule: mgmtnetwork.SecurityRule{ + SecurityRulePropertiesFormat: &mgmtnetwork.SecurityRulePropertiesFormat{ + SourceAddressPrefix: &wildcard, + DestinationAddressPrefix: &master, + }, + }, + want: true, + }, + { + name: "invalid - source any, destination worker (prefixes)", + rule: mgmtnetwork.SecurityRule{ + SecurityRulePropertiesFormat: &mgmtnetwork.SecurityRulePropertiesFormat{ + SourceAddressPrefix: &wildcard, + DestinationAddressPrefixes: &workers, + }, + }, + want: true, + }, + { + name: "invalid - source any, destination any", + rule: mgmtnetwork.SecurityRule{ + SecurityRulePropertiesFormat: &mgmtnetwork.SecurityRulePropertiesFormat{ + SourceAddressPrefix: &wildcard, + DestinationAddressPrefix: &wildcard, + }, + }, + want: true, + }, + { + name: "invalid - source master, destination vnet", + rule: mgmtnetwork.SecurityRule{ + SecurityRulePropertiesFormat: &mgmtnetwork.SecurityRulePropertiesFormat{ + SourceAddressPrefix: &master, + DestinationAddressPrefix: &vnet, + }, + }, + want: true, + }, + { + name: "invalid - source vnet, destination master (prefixes)", + rule: mgmtnetwork.SecurityRule{ + SecurityRulePropertiesFormat: &mgmtnetwork.SecurityRulePropertiesFormat{ + SourceAddressPrefix: &vnet, + DestinationAddressPrefixes: &masters, + }, + }, + want: true, + }, + { + name: "invalid - source worker, destination vnet", + rule: mgmtnetwork.SecurityRule{ + SecurityRulePropertiesFormat: &mgmtnetwork.SecurityRulePropertiesFormat{ + SourceAddressPrefix: &worker, + DestinationAddressPrefix: &vnet, + }, + }, + want: true, + }, + { + name: "invalid - source vnet, destination worker", + rule: mgmtnetwork.SecurityRule{ + SecurityRulePropertiesFormat: &mgmtnetwork.SecurityRulePropertiesFormat{ + SourceAddressPrefix: &vnet, + DestinationAddressPrefix: &worker, + }, + }, + want: true, + }, + { + name: "invalid - source vnet, destination any", + rule: mgmtnetwork.SecurityRule{ + SecurityRulePropertiesFormat: &mgmtnetwork.SecurityRulePropertiesFormat{ + SourceAddressPrefix: &vnet, + DestinationAddressPrefix: &wildcard, + }, + }, + want: true, + }, + { + name: "invalid - source any, destination vnet", + rule: mgmtnetwork.SecurityRule{ + SecurityRulePropertiesFormat: &mgmtnetwork.SecurityRulePropertiesFormat{ + SourceAddressPrefix: &wildcard, + DestinationAddressPrefix: &vnet, + }, + }, + want: true, + }, + { + name: "invalid - source vnet, destination vnet", + rule: mgmtnetwork.SecurityRule{ + SecurityRulePropertiesFormat: &mgmtnetwork.SecurityRulePropertiesFormat{ + SourceAddressPrefix: &vnet, + DestinationAddressPrefix: &vnet, + }, + }, + want: true, + }, + { + name: "valid - source nothing, destination vnet", + rule: mgmtnetwork.SecurityRule{ + SecurityRulePropertiesFormat: &mgmtnetwork.SecurityRulePropertiesFormat{ + SourceAddressPrefix: ¬hing, + DestinationAddressPrefix: &vnet, + }, + }, + want: false, + }, + { + name: "valid - source vnet, destination nothing", + rule: mgmtnetwork.SecurityRule{ + SecurityRulePropertiesFormat: &mgmtnetwork.SecurityRulePropertiesFormat{ + SourceAddressPrefix: &vnet, + DestinationAddressPrefix: ¬hing, + }, + }, + want: false, + }, + { + name: "valid - source any, destination nothing", + rule: mgmtnetwork.SecurityRule{ + SecurityRulePropertiesFormat: &mgmtnetwork.SecurityRulePropertiesFormat{ + SourceAddressPrefix: &wildcard, + DestinationAddressPrefix: ¬hing, + }, + }, + want: false, + }, + { + name: "valid - source nothing, destination any", + rule: mgmtnetwork.SecurityRule{ + SecurityRulePropertiesFormat: &mgmtnetwork.SecurityRulePropertiesFormat{ + SourceAddressPrefix: ¬hing, + DestinationAddressPrefix: &wildcard, + }, + }, + want: false, + }, + } { + t.Run(tt.name, func(t *testing.T) { + c := newRuleChecker(log, masterPrefix, workerPrefixes, &tt.rule) + got := c.isInvalidDenyRule() + if got != tt.want { + t.Errorf("want %t, got %t", tt.want, got) + } + }) + } +}