Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Network Storage Account Lockdown #1865

Merged
merged 15 commits into from
Feb 24, 2022
Merged

Network Storage Account Lockdown #1865

Show file tree
Hide file tree
Changes from 13 commits
Commits
Show all changes
15 commits
Select commit Hold shift + click to select a range
c1662ed
add operator storage acc and endpoints reconcilers
mjudeikis Sep 10, 2021
6ea8de1
operator tests
mjudeikis Sep 10, 2021
ac3c474
storageacc handling for install/update
mjudeikis Sep 10, 2021
9bf8b32
generate
mjudeikis Sep 10, 2021
5e1ad15
vendor
mjudeikis Sep 27, 2021
507c785
review feedback
mjudeikis Sep 27, 2021
cee82e5
Add dev env rules exception
Oct 12, 2021
a3ae37f
Fix tests
Dec 3, 2021
32d2dcf
Add operator flags
Jan 20, 2022
797bf00
Fix tests
Jan 20, 2022
c8d55c6
Change operator flags
Jan 21, 2022
10fde74
Addressing feedback
Jan 25, 2022
bf5ce15
Operator flag tests
Feb 15, 2022
7b68ab7
Addressing feedback
Feb 16, 2022
f8257e6
update cluster spec
Feb 23, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
11 changes: 11 additions & 0 deletions cmd/aro/operator.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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"
Expand All @@ -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"
Expand Down Expand Up @@ -99,6 +101,10 @@ func operator(ctx context.Context, log *logrus.Entry) error {
if err != nil {
return err
}
imageregistryclient, 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 {
Expand Down Expand Up @@ -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, imageregistryclient)).SetupWithManager(mgr); err != nil {
return fmt.Errorf("unable to create controller %s: %v", controllers.StorageAccountsControllerName, err)
}
}

if err = (checker.NewReconciler(
Expand Down
8 changes: 8 additions & 0 deletions deploy/rp-development-predeploy.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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"
Expand Down
14 changes: 14 additions & 0 deletions deploy/rp-production-predeploy.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -205,6 +205,12 @@
"locations": [
"*"
]
},
{
"service": "Microsoft.Storage",
"locations": [
"*"
]
}
]
},
Expand Down Expand Up @@ -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"
Expand Down
6 changes: 5 additions & 1 deletion pkg/api/defaults.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -51,7 +51,9 @@ const FLAG_FALSE string = "false"
var DefaultOperatorFlags OperatorFlags = OperatorFlags{
"aro.alertwebhook.enabled": FLAG_TRUE,

"aro.azuresubnets.enabled": FLAG_TRUE,
"aro.azuresubnets.enabled": FLAG_TRUE,
25region marked this conversation as resolved.
Show resolved Hide resolved
"aro.azuresubnets.nsg.managed": FLAG_TRUE,
25region marked this conversation as resolved.
Show resolved Hide resolved
"aro.azuresubnets.serviceendpoint.managed": FLAG_TRUE,

"aro.banner.enabled": FLAG_FALSE,

Expand All @@ -78,6 +80,8 @@ var DefaultOperatorFlags OperatorFlags = OperatorFlags{

"aro.routefix.enabled": FLAG_TRUE,

"aro.storageaccounts.enabled": FLAG_TRUE,

"aro.workaround.enabled": FLAG_TRUE,
}

Expand Down
11 changes: 11 additions & 0 deletions pkg/api/subnets.go
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,11 @@
package api

// Copyright (c) Microsoft Corporation.
// Licensed under the Apache License 2.0.

var (
SubnetsEndpoints = []string{
"Microsoft.ContainerRegistry",
"Microsoft.Storage",
}
)
6 changes: 6 additions & 0 deletions pkg/cluster/adminupdate_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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]",
Expand Down Expand Up @@ -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]",
Expand Down
20 changes: 10 additions & 10 deletions pkg/cluster/cluster.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
imageregistryclient imageregistryclient.Interface
25region marked this conversation as resolved.
Show resolved Hide resolved
}

// New returns a cluster manager
Expand Down
4 changes: 2 additions & 2 deletions pkg/cluster/deploystorage.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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(),
Expand Down
92 changes: 81 additions & 11 deletions pkg/cluster/deploystorage_resources.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -74,19 +74,89 @@ 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.
25region marked this conversation as resolved.
Show resolved Hide resolved
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,
25region marked this conversation as resolved.
Show resolved Hide resolved
},
}

// Prod includes a gateway rule as well
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,
25region marked this conversation as resolved.
Show resolved Hide resolved
})
}

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(mjudeikis): Move to development on VPN so we can make this IPRule
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

gives me simplysecure smells... @swiencki fyi

25region marked this conversation as resolved.
Show resolved Hide resolved
if m.env.IsLocalDevelopmentMode() {
sa.NetworkRuleSet.DefaultAction = mgmtstorage.DefaultActionAllow
}
// When migrating storage accounts for old cluster we are not able to change
// encryption. For this we have this encryption flag. We not gonna add this
// retrospectively to old clusters
25region marked this conversation as resolved.
Show resolved Hide resolved
if encrypted {
bennerv marked this conversation as resolved.
Show resolved Hide resolved
25region marked this conversation as resolved.
Show resolved Hide resolved
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"),
}
}
Expand Down
6 changes: 5 additions & 1 deletion pkg/cluster/install.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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)),
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If the Operator does it, do we strictly need to do this in the AdminUpdate?

Copy link
Collaborator

@bennerv bennerv Jan 26, 2022 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This isn't necessarily needed, but it may eliminate some errors upon enabling the storage lockdown on the controller for existing clusters.

Over time, desired state would look good, but if we removed this step during the first controller run and the controller which sets storage account virtual network rules attempts to run first, the storage account will fail to update because storage endpoints must first be set on the subnet.

This will stop an SRE from wondering if this is a normal error with the controller, or if it's a first time reconciliation issue (noise)

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
Expand Down Expand Up @@ -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)
Expand Down Expand Up @@ -288,7 +292,7 @@ func (m *manager) initializeKubernetesClients(ctx context.Context) error {
return err
}

m.registryclient, err = imageregistryclient.NewForConfig(restConfig)
m.imageregistryclient, err = imageregistryclient.NewForConfig(restConfig)
return err
}

Expand Down
Loading