Skip to content

Commit

Permalink
Revert "Add nsg enhancement permission validation for FP/Cluster SPs (#…
Browse files Browse the repository at this point in the history 
…2969)"

This reverts commit f87f198.
  • Loading branch information
@bizz001
bizz001 committed Jul 21, 2023
1 parent 678109c commit d5fa4ff
Show file tree
Hide file tree
Showing 4 changed files with 0 additions and 254 deletions.
61 changes: 0 additions & 61 deletions pkg/api/validate/dynamic/dynamic.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -38,7 +38,6 @@ var (
errMsgOriginalNSGNotAttached = "The provided subnet '%s' is invalid: must have network security group '%s' attached."
errMsgNSGNotAttached = "The provided subnet '%s' is invalid: must have a network security group attached."
errMsgNSGNotProperlyAttached = "When the enable-preconfigured-nsg option is specified, both the master and worker subnets should have network security groups (NSG) attached to them before starting the cluster installation."
errMsgSPHasNoRequiredPermissionsOnNSG = "The %s service principal (Application ID: %s) does not have Network Contributor role on network security group '%s'. This is required when the enable-preconfigured-nsg option is specified."
errMsgSubnetNotFound = "The provided subnet '%s' could not be found."
errMsgSubnetNotInSucceededState = "The provided subnet '%s' is not in a Succeeded state"
errMsgSubnetInvalidSize = "The provided subnet '%s' is invalid: must be /27 or larger."
Expand Down Expand Up @@ -72,7 +71,6 @@ type Dynamic interface {
ValidateSubnets(ctx context.Context, oc *api.OpenShiftCluster, subnets []Subnet) error
ValidateDiskEncryptionSets(ctx context.Context, oc *api.OpenShiftCluster) error
ValidateEncryptionAtHost(ctx context.Context, oc *api.OpenShiftCluster) error
ValidatePreConfiguredNSGs(ctx context.Context, oc *api.OpenShiftCluster, subnets []Subnet) error
}

type dynamic struct {
Expand Down Expand Up @@ -783,65 +781,6 @@ func (dv *dynamic) ValidateSubnets(ctx context.Context, oc *api.OpenShiftCluster
return nil
}

func (dv *dynamic) ValidatePreConfiguredNSGs(ctx context.Context, oc *api.OpenShiftCluster, subnets []Subnet) error {
dv.log.Print("ValidatePreConfiguredNSGs")

if oc.Properties.NetworkProfile.PreconfiguredNSG != api.PreconfiguredNSGEnabled {
return nil // exit early
}

subnetByID, err := dv.createSubnetMapByID(ctx, subnets)
if err != nil {
return err
}

for _, s := range subnetByID {
nsgID := s.NetworkSecurityGroup.ID
if nsgID == nil || *nsgID == "" {
return api.NewCloudError(
http.StatusBadRequest,
api.CloudErrorCodeNotFound,
"",
errMsgNSGNotProperlyAttached,
)
}

if err := dv.validateNSGPermissions(ctx, *nsgID); err != nil {
return err
}
}
return nil
}

func (dv *dynamic) validateNSGPermissions(ctx context.Context, nsgID string) error {
nsg, err := azure.ParseResourceID(nsgID)
if err != nil {
return err
}

err = dv.validateActions(ctx, &nsg, []string{
"Microsoft.Network/networkSecurityGroups/join/action",
})

if err == wait.ErrWaitTimeout {
errCode := api.CloudErrorCodeInvalidResourceProviderPermissions
if dv.authorizerType == AuthorizerClusterServicePrincipal {
errCode = api.CloudErrorCodeInvalidServicePrincipalPermissions
}
return api.NewCloudError(
http.StatusBadRequest,
errCode,
"",
errMsgSPHasNoRequiredPermissionsOnNSG,
dv.authorizerType,
dv.appID,
nsgID,
)
}

return err
}

func isTheSameNSG(found, inDB string) bool {
return strings.EqualFold(found, inDB)
}
Expand Down
169 changes: 0 additions & 169 deletions pkg/api/validate/dynamic/dynamic_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -38,7 +38,6 @@ var (
masterSubnet = vnetID + "/subnet/masterSubnet"
workerSubnet = vnetID + "/subnet/workerSubnet"
masterSubnetPath = "properties.masterProfile.subnetId"
workerSubnetPath = "properties.workerProfile.subnetId"
masterRtID = resourceGroupID + "/providers/Microsoft.Network/routeTables/masterRt"
workerRtID = resourceGroupID + "/providers/Microsoft.Network/routeTables/workerRt"
masterNgID = resourceGroupID + "/providers/Microsoft.Network/natGateways/masterNg"
Expand Down Expand Up @@ -1126,7 +1125,6 @@ var (
func mockTokenCredential(tokenCred *mock_azcore.MockTokenCredential) {
tokenCred.EXPECT().
GetToken(gomock.Any(), gomock.Any()).
AnyTimes().
Return(mockAccessToken, nil)
}

Expand Down Expand Up @@ -1729,170 +1727,3 @@ func TestCheckBYONsg(t *testing.T) {
})
}
}

var (
canJoinNSG = remotepdp.AuthorizationDecisionResponse{
Value: []remotepdp.AuthorizationDecision{
{
ActionId: "Microsoft.Network/networkSecurityGroups/join/action",
AccessDecision: remotepdp.Allowed},
},
}

cannotJoinNSG = remotepdp.AuthorizationDecisionResponse{
Value: []remotepdp.AuthorizationDecision{
{
ActionId: "Microsoft.Network/networkSecurityGroups/join/action",
AccessDecision: remotepdp.NotAllowed},
},
}
)

func TestValidatePreconfiguredNSGPermissions(t *testing.T) {
ctx := context.Background()
for _, tt := range []struct {
name string
modifyOC func(*api.OpenShiftCluster)
checkAccessMocks func(context.CancelFunc, *mock_remotepdp.MockRemotePDPClient, *mock_azcore.MockTokenCredential)
vnetMocks func(*mock_network.MockVirtualNetworksClient, mgmtnetwork.VirtualNetwork)
wantErr string
}{
{
name: "pass: skip when preconfiguredNSG is not enabled",
modifyOC: func(oc *api.OpenShiftCluster) {
oc.Properties.NetworkProfile.PreconfiguredNSG = api.PreconfiguredNSGDisabled
},
},
{
name: "fail: sp doesn't have the permission on all NSGs",
modifyOC: func(oc *api.OpenShiftCluster) {
oc.Properties.NetworkProfile.PreconfiguredNSG = api.PreconfiguredNSGEnabled
},
vnetMocks: func(vnetClient *mock_network.MockVirtualNetworksClient, vnet mgmtnetwork.VirtualNetwork) {
vnetClient.EXPECT().
Get(gomock.Any(), resourceGroupName, vnetName, "").
AnyTimes().
Return(vnet, nil)
},
checkAccessMocks: func(cancel context.CancelFunc, pdpClient *mock_remotepdp.MockRemotePDPClient, tokenCred *mock_azcore.MockTokenCredential) {
mockTokenCredential(tokenCred)
pdpClient.EXPECT().CheckAccess(gomock.Any(), gomock.Any()).
DoAndReturn(func(_ context.Context, authReq remotepdp.AuthorizationRequest) (*remotepdp.AuthorizationDecisionResponse, error) {
cancel() // wait.PollImmediateUntil will always be invoked at least once
switch authReq.Resource.Id {
case masterNSGv1:
return &canJoinNSG, nil
case workerNSGv1:
return &cannotJoinNSG, nil
}
return &cannotJoinNSG, nil
},
).AnyTimes()
},
wantErr: "400: InvalidServicePrincipalPermissions: : The cluster service principal (Application ID: fff51942-b1f9-4119-9453-aaa922259eb7) does not have Network Contributor role on network security group '/subscriptions/0000000-0000-0000-0000-000000000000/resourceGroups/testGroup/providers/Microsoft.Network/networkSecurityGroups/aro-node-nsg'. This is required when the enable-preconfigured-nsg option is specified.",
},
{
name: "pass: sp has the required permission on the NSG",
modifyOC: func(oc *api.OpenShiftCluster) {
oc.Properties.NetworkProfile.PreconfiguredNSG = api.PreconfiguredNSGEnabled
},
vnetMocks: func(vnetClient *mock_network.MockVirtualNetworksClient, vnet mgmtnetwork.VirtualNetwork) {
vnetClient.EXPECT().
Get(gomock.Any(), resourceGroupName, vnetName, "").
AnyTimes().
Return(vnet, nil)
},
checkAccessMocks: func(cancel context.CancelFunc, pdpClient *mock_remotepdp.MockRemotePDPClient, tokenCred *mock_azcore.MockTokenCredential) {
mockTokenCredential(tokenCred)
pdpClient.EXPECT().CheckAccess(gomock.Any(), gomock.Any()).
Do(func(_, _ interface{}) {
cancel()
}).
Return(&canJoinNSG, nil).
AnyTimes()
},
},
} {
t.Run(tt.name, func(t *testing.T) {
controller := gomock.NewController(t)
defer controller.Finish()

ctx, cancel := context.WithCancel(ctx)
defer cancel()

oc := &api.OpenShiftCluster{
Properties: api.OpenShiftClusterProperties{
ClusterProfile: api.ClusterProfile{
ResourceGroupID: resourceGroupID,
},
},
}
vnet := mgmtnetwork.VirtualNetwork{
ID: &vnetID,
VirtualNetworkPropertiesFormat: &mgmtnetwork.VirtualNetworkPropertiesFormat{
Subnets: &[]mgmtnetwork.Subnet{
{
ID: &masterSubnet,
SubnetPropertiesFormat: &mgmtnetwork.SubnetPropertiesFormat{
AddressPrefix: to.StringPtr("10.0.0.0/24"),
NetworkSecurityGroup: &mgmtnetwork.SecurityGroup{
ID: &masterNSGv1,
},
ProvisioningState: mgmtnetwork.Succeeded,
},
},
{
ID: &workerSubnet,
SubnetPropertiesFormat: &mgmtnetwork.SubnetPropertiesFormat{
AddressPrefix: to.StringPtr("10.0.1.0/24"),
NetworkSecurityGroup: &mgmtnetwork.SecurityGroup{
ID: &workerNSGv1,
},
ProvisioningState: mgmtnetwork.Succeeded,
},
},
},
},
}

if tt.modifyOC != nil {
tt.modifyOC(oc)
}

vnetClient := mock_network.NewMockVirtualNetworksClient(controller)

if tt.vnetMocks != nil {
tt.vnetMocks(vnetClient, vnet)
}

tokenCred := mock_azcore.NewMockTokenCredential(controller)
pdpClient := mock_remotepdp.NewMockRemotePDPClient(controller)

if tt.checkAccessMocks != nil {
tt.checkAccessMocks(cancel, pdpClient, tokenCred)
}

dv := &dynamic{
azEnv: &azureclient.PublicCloud,
appID: "fff51942-b1f9-4119-9453-aaa922259eb7",
authorizerType: AuthorizerClusterServicePrincipal,
virtualNetworks: vnetClient,
pdpClient: pdpClient,
checkAccessSubjectInfoCred: tokenCred,
log: logrus.NewEntry(logrus.StandardLogger()),
}

subnets := []Subnet{
{ID: masterSubnet,
Path: masterSubnetPath},
{
ID: workerSubnet,
Path: workerSubnetPath,
},
}

err := dv.ValidatePreConfiguredNSGs(ctx, oc, subnets)
utilerror.AssertErrorMessage(t, err, tt.wantErr)
})
}
}
10 changes: 0 additions & 10 deletions pkg/api/validate/openshiftcluster_validatedynamic.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -199,11 +199,6 @@ func (dv *openShiftClusterDynamicValidator) Dynamic(ctx context.Context) error {
return err
}

err = fpDynamic.ValidatePreConfiguredNSGs(ctx, dv.oc, subnets)
if err != nil {
return err
}

tenantID := dv.subscriptionDoc.Subscription.Properties.TenantID
options := dv.env.Environment().ClientSecretCredentialOptions()
spTokenCredential, err := azidentity.NewClientSecretCredential(
Expand Down Expand Up @@ -263,10 +258,5 @@ func (dv *openShiftClusterDynamicValidator) Dynamic(ctx context.Context) error {
return err
}

err = spDynamic.ValidatePreConfiguredNSGs(ctx, dv.oc, subnets)
if err != nil {
return err
}

return nil
}
14 changes: 0 additions & 14 deletions pkg/util/mocks/dynamic/dynamic.go
View file

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

0 comments on commit d5fa4ff

Please sign in to comment.