Network Storage Account Lockdown

cmd/aro/operator.go

@@ -10,6 +10,7 @@ import (
 
 	configclient "github.com/openshift/client-go/config/clientset/versioned"
 	consoleclient "github.com/openshift/client-go/console/clientset/versioned"
+	imageregistryclient "github.com/openshift/client-go/imageregistry/clientset/versioned"
 	securityclient "github.com/openshift/client-go/security/clientset/versioned"
 	maoclient "github.com/openshift/machine-api-operator/pkg/generated/clientset/versioned"
 	mcoclient "github.com/openshift/machine-config-operator/pkg/generated/clientset/versioned"
@@ -36,6 +37,7 @@ import (
 	"github.com/Azure/ARO-RP/pkg/operator/controllers/pullsecret"
 	"github.com/Azure/ARO-RP/pkg/operator/controllers/rbac"
 	"github.com/Azure/ARO-RP/pkg/operator/controllers/routefix"
+	"github.com/Azure/ARO-RP/pkg/operator/controllers/storageaccounts"
 	"github.com/Azure/ARO-RP/pkg/operator/controllers/subnets"
 	"github.com/Azure/ARO-RP/pkg/operator/controllers/workaround"
 	"github.com/Azure/ARO-RP/pkg/util/dynamichelper"
@@ -99,6 +101,10 @@ func operator(ctx context.Context, log *logrus.Entry) error {
 	if err != nil {
 		return err
 	}
+	imageregistrycli, err := imageregistryclient.NewForConfig(restConfig)
+	if err != nil {
+		return err
+	}
 	// TODO (NE): dh is sometimes passed, sometimes created later. Can we standardize?
 	dh, err := dynamichelper.New(log, restConfig)
 	if err != nil {
@@ -191,6 +197,11 @@ func operator(ctx context.Context, log *logrus.Entry) error {
 			arocli, configcli)).SetupWithManager(mgr); err != nil {
 			return fmt.Errorf("unable to create controller ImageConfig: %v", err)
 		}
+		if err = (storageaccounts.NewReconciler(
+			log.WithField("controller", controllers.StorageAccountsControllerName),
+			arocli, maocli, kubernetescli, imageregistrycli)).SetupWithManager(mgr); err != nil {
+			return fmt.Errorf("unable to create controller %s: %v", controllers.StorageAccountsControllerName, err)
+		}
 	}
 
 	if err = (checker.NewReconciler(

deploy/rp-development-predeploy.json

@@ -103,6 +103,14 @@
                                 "id": "[resourceId('Microsoft.Network/networkSecurityGroups', 'rp-pe-nsg')]",
                                 "tags": null
                             },
+                            "serviceEndpoints": [
+                                {
+                                    "service": "Microsoft.Storage",
+                                    "locations": [
+                                        "*"
+                                    ]
+                                }
+                            ],
                             "privateEndpointNetworkPolicies": "Disabled"
                         },
                         "name": "rp-pe-subnet"

deploy/rp-production-predeploy.json

@@ -205,6 +205,12 @@
                                     "locations": [
                                         "*"
                                     ]
+                                },
+                                {
+                                    "service": "Microsoft.Storage",
+                                    "locations": [
+                                        "*"
+                                    ]
                                 }
                             ]
                         },
@@ -235,6 +241,14 @@
                                 "id": "[resourceId('Microsoft.Network/networkSecurityGroups', 'rp-pe-nsg')]",
                                 "tags": null
                             },
+                            "serviceEndpoints": [
+                                {
+                                    "service": "Microsoft.Storage",
+                                    "locations": [
+                                        "*"
+                                    ]
+                                }
+                            ],
                             "privateEndpointNetworkPolicies": "Disabled"
                         },
                         "name": "rp-pe-subnet"

pkg/api/defaults.go

@@ -49,36 +49,25 @@ const FLAG_FALSE string = "false"
 
 // Default flags for new clusters & ones that have not been AdminUpdated
 var DefaultOperatorFlags OperatorFlags = OperatorFlags{
-	"aro.alertwebhook.enabled": FLAG_TRUE,
-
-	"aro.azuresubnets.enabled": FLAG_TRUE,
-
-	"aro.banner.enabled": FLAG_FALSE,
-
-	"aro.checker.enabled": FLAG_TRUE,
-
-	"aro.dnsmasq.enabled": FLAG_TRUE,
-
-	"aro.genevalogging.enabled": FLAG_TRUE,
-
-	"aro.imageconfig.enabled": FLAG_TRUE,
-
-	"aro.machine.enabled": FLAG_TRUE,
-
-	"aro.machineset.enabled": FLAG_TRUE,
-
-	"aro.monitoring.enabled": FLAG_TRUE,
-
-	"aro.nodedrainer.enabled": FLAG_TRUE,
-
-	"aro.pullsecret.enabled": FLAG_TRUE,
-	"aro.pullsecret.managed": FLAG_TRUE,
-
-	"aro.rbac.enabled": FLAG_TRUE,
-
-	"aro.routefix.enabled": FLAG_TRUE,
-
-	"aro.workaround.enabled": FLAG_TRUE,
+	"aro.alertwebhook.enabled":                 FLAG_TRUE,
+	"aro.azuresubnets.enabled":                 FLAG_TRUE,
+	"aro.azuresubnets.nsg.managed":             FLAG_TRUE,
+	"aro.azuresubnets.serviceendpoint.managed": FLAG_TRUE,
+	"aro.banner.enabled":                       FLAG_FALSE,
+	"aro.checker.enabled":                      FLAG_TRUE,
+	"aro.dnsmasq.enabled":                      FLAG_TRUE,
+	"aro.genevalogging.enabled":                FLAG_TRUE,
+	"aro.imageconfig.enabled":                  FLAG_TRUE,
+	"aro.machine.enabled":                      FLAG_TRUE,
+	"aro.machineset.enabled":                   FLAG_TRUE,
+	"aro.monitoring.enabled":                   FLAG_TRUE,
+	"aro.nodedrainer.enabled":                  FLAG_TRUE,
+	"aro.pullsecret.enabled":                   FLAG_TRUE,
+	"aro.pullsecret.managed":                   FLAG_TRUE,
+	"aro.rbac.enabled":                         FLAG_TRUE,
+	"aro.routefix.enabled":                     FLAG_TRUE,
+	"aro.storageaccounts.enabled":              FLAG_TRUE,
+	"aro.workaround.enabled":                   FLAG_TRUE,
 }
 
 func (d OperatorFlags) Copy() OperatorFlags {

pkg/api/subnets.go

@@ -0,0 +1,11 @@
+package api
+
+// Copyright (c) Microsoft Corporation.
+// Licensed under the Apache License 2.0.
+
+var (
+	SubnetsEndpoints = []string{
+		"Microsoft.ContainerRegistry",
+		"Microsoft.Storage",
+	}
+)

pkg/cluster/adminupdate_test.go

@@ -67,6 +67,9 @@ func TestAdminUpdateSteps(t *testing.T) {
 				"[Action fixInfraID-fm]",
 				"[AuthorizationRefreshingAction [Action ensureResourceGroup-fm]]",
 				"[Action createOrUpdateDenyAssignment-fm]",
+				"[AuthorizationRefreshingAction [Action enableServiceEndpoints-fm]]",
+				"[Action populateRegistryStorageAccountName-fm]",
+				"[Action migrateStorageAccounts-fm]",
 				"[Action fixSSH-fm]",
 				"[Action populateDatabaseIntIP-fm]",
 				"[Action startVMs-fm]",
@@ -102,6 +105,9 @@ func TestAdminUpdateSteps(t *testing.T) {
 				"[Action fixInfraID-fm]",
 				"[AuthorizationRefreshingAction [Action ensureResourceGroup-fm]]",
 				"[Action createOrUpdateDenyAssignment-fm]",
+				"[AuthorizationRefreshingAction [Action enableServiceEndpoints-fm]]",
+				"[Action populateRegistryStorageAccountName-fm]",
+				"[Action migrateStorageAccounts-fm]",
 				"[Action fixSSH-fm]",
 				"[Action populateDatabaseIntIP-fm]",
 				"[Action startVMs-fm]",

pkg/cluster/cluster.go

@@ -80,16 +80,16 @@ type manager struct {
 	subnet  subnet.Manager
 	graph   graph.Manager
 
-	kubernetescli  kubernetes.Interface
-	extensionscli  extensionsclient.Interface
-	maocli         maoclient.Interface
-	mcocli         mcoclient.Interface
-	operatorcli    operatorclient.Interface
-	configcli      configclient.Interface
-	samplescli     samplesclient.Interface
-	securitycli    securityclient.Interface
-	arocli         aroclient.Interface
-	registryclient imageregistryclient.Interface
+	kubernetescli    kubernetes.Interface
+	extensionscli    extensionsclient.Interface
+	maocli           maoclient.Interface
+	mcocli           mcoclient.Interface
+	operatorcli      operatorclient.Interface
+	configcli        configclient.Interface
+	samplescli       samplesclient.Interface
+	securitycli      securityclient.Interface
+	arocli           aroclient.Interface
+	imageregistrycli imageregistryclient.Interface
 }
 
 // New returns a cluster manager

pkg/cluster/deploystorage.go

@@ -132,10 +132,10 @@ func (m *manager) deployStorageTemplate(ctx context.Context, installConfig *inst
 	clusterStorageAccountName := "cluster" + m.doc.OpenShiftCluster.Properties.StorageSuffix
 
 	resources := []*arm.Resource{
-		m.storageAccount(clusterStorageAccountName, installConfig.Config.Azure.Region),
+		m.storageAccount(clusterStorageAccountName, installConfig.Config.Azure.Region, true),
 		m.storageAccountBlobContainer(clusterStorageAccountName, "ignition"),
 		m.storageAccountBlobContainer(clusterStorageAccountName, "aro"),
-		m.storageAccount(m.doc.OpenShiftCluster.Properties.ImageRegistryStorageAccountName, installConfig.Config.Azure.Region),
+		m.storageAccount(m.doc.OpenShiftCluster.Properties.ImageRegistryStorageAccountName, installConfig.Config.Azure.Region, true),
 		m.storageAccountBlobContainer(m.doc.OpenShiftCluster.Properties.ImageRegistryStorageAccountName, "image-registry"),
 		m.clusterNSG(infraID, installConfig.Config.Azure.Region),
 		m.clusterServicePrincipalRBAC(),

pkg/cluster/deploystorage_resources.go

@@ -74,19 +74,93 @@ func (m *manager) clusterServicePrincipalRBAC() *arm.Resource {
 	)
 }
 
-func (m *manager) storageAccount(name, region string) *arm.Resource {
-	return &arm.Resource{
-		Resource: &mgmtstorage.Account{
-			Sku: &mgmtstorage.Sku{
-				Name: "Standard_LRS",
-			},
-			AccountProperties: &mgmtstorage.AccountProperties{
-				MinimumTLSVersion: mgmtstorage.TLS12,
+// storageAccount will return storage account resource.
+// Legacy storage accounts (public) are not encrypted and cannot be retrofitted.
+// The flag controls this behavior in update/create.
+func (m *manager) storageAccount(name, region string, encrypted bool) *arm.Resource {
+
+	virtualNetworkRules := []mgmtstorage.VirtualNetworkRule{
+		{
+			VirtualNetworkResourceID: to.StringPtr(m.doc.OpenShiftCluster.Properties.MasterProfile.SubnetID),
+			Action:                   mgmtstorage.Allow,
+		},
+		{
+			VirtualNetworkResourceID: to.StringPtr(m.doc.OpenShiftCluster.Properties.WorkerProfiles[0].SubnetID),
+			Action:                   mgmtstorage.Allow,
+		},
+		{
+			VirtualNetworkResourceID: to.StringPtr("/subscriptions/" + m.env.SubscriptionID() + "/resourceGroups/" + m.env.ResourceGroup() + "/providers/Microsoft.Network/virtualNetworks/rp-pe-vnet-001/subnets/rp-pe-subnet"),
+			Action:                   mgmtstorage.Allow,
+		},
+		{
+			VirtualNetworkResourceID: to.StringPtr("/subscriptions/" + m.env.SubscriptionID() + "/resourceGroups/" + m.env.ResourceGroup() + "/providers/Microsoft.Network/virtualNetworks/rp-vnet/subnets/rp-subnet"),
+			Action:                   mgmtstorage.Allow,
+		},
+	}
+
+	// Prod includes a gateway rule as well
+	// Once we reach a PLS limit (1000) within a vnet , we may need some refactoring here
+	// https://docs.microsoft.com/en-us/azure/azure-resource-manager/management/azure-subscription-service-limits#private-link-limits
+	if !m.env.IsLocalDevelopmentMode() {
+		virtualNetworkRules = append(virtualNetworkRules, mgmtstorage.VirtualNetworkRule{
+			VirtualNetworkResourceID: to.StringPtr("/subscriptions/" + m.env.SubscriptionID() + "/resourceGroups/" + m.env.GatewayResourceGroup() + "/providers/Microsoft.Network/virtualNetworks/gateway-vnet/subnets/gateway-subnet"),
+			Action:                   mgmtstorage.Allow,
+		})
+	}
+
+	sa := &mgmtstorage.Account{
+		Kind: mgmtstorage.StorageV2,
+		Sku: &mgmtstorage.Sku{
+			Name: "Standard_LRS",
+		},
+		AccountProperties: &mgmtstorage.AccountProperties{
+			AllowBlobPublicAccess:  to.BoolPtr(false),
+			EnableHTTPSTrafficOnly: to.BoolPtr(true),
+			MinimumTLSVersion:      mgmtstorage.TLS12,
+			NetworkRuleSet: &mgmtstorage.NetworkRuleSet{
+				Bypass:              mgmtstorage.AzureServices,
+				VirtualNetworkRules: &virtualNetworkRules,
+				DefaultAction:       "Deny",
 			},
-			Name:     &name,
-			Location: &region,
-			Type:     to.StringPtr("Microsoft.Storage/storageAccounts"),
 		},
+		Name:     &name,
+		Location: &region,
+		Type:     to.StringPtr("Microsoft.Storage/storageAccounts"),
+	}
+
+	// In development API calls originates from user laptop so we allow all.
+	// TODO: Move to development on VPN so we can make this IPRule.  Will be done as part of Simply secure v2 work
+	if m.env.IsLocalDevelopmentMode() {
+		sa.NetworkRuleSet.DefaultAction = mgmtstorage.DefaultActionAllow
+	}
+	// When migrating storage accounts for old clusters we are not able to change
+	// encryption which is why we have this encryption flag. We will not add this
+	// retrospectively to old clusters
+	// If a storage account already has encryption enabled and the encrypted
+	// bool is set to false, it will still maintain the encryption on the storage account.
+	if encrypted {
+		sa.AccountProperties.Encryption = &mgmtstorage.Encryption{
+			RequireInfrastructureEncryption: to.BoolPtr(true),
+			Services: &mgmtstorage.EncryptionServices{
+				Blob: &mgmtstorage.EncryptionService{
+					KeyType: mgmtstorage.KeyTypeAccount,
+				},
+				File: &mgmtstorage.EncryptionService{
+					KeyType: mgmtstorage.KeyTypeAccount,
+				},
+				Table: &mgmtstorage.EncryptionService{
+					KeyType: mgmtstorage.KeyTypeAccount,
+				},
+				Queue: &mgmtstorage.EncryptionService{
+					KeyType: mgmtstorage.KeyTypeAccount,
+				},
+			},
+			KeySource: mgmtstorage.KeySourceMicrosoftStorage,
+		}
+	}
+
+	return &arm.Resource{
+		Resource:   sa,
 		APIVersion: azureclient.APIVersion("Microsoft.Storage"),
 	}
 }

pkg/cluster/install.go

@@ -52,6 +52,9 @@ func (m *manager) adminUpdate() []steps.Step {
 		toRun = append(toRun,
 			steps.AuthorizationRefreshingAction(m.fpAuthorizer, steps.Action(m.ensureResourceGroup)), // re-create RP RBAC if needed after tenant migration
 			steps.Action(m.createOrUpdateDenyAssignment),
+			steps.AuthorizationRefreshingAction(m.fpAuthorizer, steps.Action(m.enableServiceEndpoints)),
+			steps.Action(m.populateRegistryStorageAccountName), // must go before migrateStorageAccounts
+			steps.Action(m.migrateStorageAccounts),
 			steps.Action(m.fixSSH),
 			steps.Action(m.populateDatabaseIntIP),
 			//steps.Action(m.removePrivateDNSZone), // TODO(mj): re-enable once we communicate this out
@@ -142,6 +145,7 @@ func (m *manager) Install(ctx context.Context) error {
 				return m.ensureInfraID(ctx, installConfig)
 			}),
 			steps.AuthorizationRefreshingAction(m.fpAuthorizer, steps.Action(m.ensureResourceGroup)),
+			steps.AuthorizationRefreshingAction(m.fpAuthorizer, steps.Action(m.enableServiceEndpoints)),
 			steps.AuthorizationRefreshingAction(m.fpAuthorizer, steps.Action(m.setMasterSubnetPolicies)),
 			steps.AuthorizationRefreshingAction(m.fpAuthorizer, steps.Action(func(ctx context.Context) error {
 				return m.deployStorageTemplate(ctx, installConfig)
@@ -288,7 +292,7 @@ func (m *manager) initializeKubernetesClients(ctx context.Context) error {
 		return err
 	}
 
-	m.registryclient, err = imageregistryclient.NewForConfig(restConfig)
+	m.imageregistrycli, err = imageregistryclient.NewForConfig(restConfig)
 	return err
 }