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

merged 15 commits into from
Feb 24, 2022

Conversation

25region
Copy link
Contributor

@25region 25region commented Dec 2, 2021

Supersedes #1657

Which issue this PR addresses:

Enables storage account lockdown.
Quite a big change so early reviews would be good. Let me know if people want separate review call for this one.

What this PR does / why we need it:

This pr will do few things to make storage accounts side more secure:

  1. Deploy all storage accounts encrypted (fedramp)
  2. Add firewalls rules for RP and customer subnets on storage accounts
  3. Add controller to make sure firewall rules and service endpoints are configures right.
  4. Remove validation for service endpoint in favor of install step
  5. Add AdminPutOrPatch to migrate existing infrastructure to locked setup

Split:
#1663 - Subnet related changes in the operator mostly
#1664 - client updates
#1723 - Registry account populate

TODO:

  • Refactor subnet controller to be extendable
  • Add firewall reconcile
  • Add prepopulate storage account name in db for old clusters
  • reconcile old storage accounts
  • Add controller to enable service endpoints on subnets
  • Add controller tests
    - [ ] Add ARM feature flag for this so we can test Tempted just to rollout...

Test plan for issue:

Unit tests and mocks

-->

Is there any documentation that needs to be updated for this PR?

Yes

@25region 25region added hold Hold priority-low Low priority issue or pull request size-large Size large ready-for-review and removed ready-for-review labels Dec 2, 2021
@25region 25region force-pushed the storage.lockdown.final branch 2 times, most recently from b1955af to a0dc5a2 Compare December 6, 2021 16:18
@github-actions
Copy link

Please rebase pull request.

@github-actions github-actions bot added the needs-rebase branch needs a rebase label Jan 14, 2022
@github-actions github-actions bot removed the needs-rebase branch needs a rebase label Jan 18, 2022
hawkowl
hawkowl reviewed Jan 25, 2022
View reviewed changes
pkg/cluster/storageaccounts.go
)

// enableServiceEndpoints should enable service endpoints on
// subnets for storage account access
func (m *manager) enableServiceEndpoints(ctx context.Context) error {
Copy link
Collaborator

Choose a reason for hiding this comment

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

I don't know about doing this automatically -- I would feel a lot better if this was something we put behind a flag. Maybe not the OperatorFlags, since they are strictly for the operator, but... hmm.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Should we have this only work when we enable storageaccounts new logic? If we create private storage accounts, there will be no access without Service Endpoints.

hawkowl
hawkowl reviewed Jan 25, 2022
View reviewed changes
pkg/api/defaults.go Outdated Show resolved Hide resolved
hawkowl
hawkowl reviewed Jan 25, 2022
View reviewed changes
test/e2e/operator.go Outdated Show resolved Hide resolved
hawkowl
hawkowl reviewed Jan 25, 2022
View reviewed changes
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)),
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)

s-amann
s-amann previously approved these changes Feb 15, 2022
View reviewed changes
@ross-bryan ross-bryan added priority-high High priority issue or pull request and removed priority-low Low priority issue or pull request labels Feb 15, 2022
ross-bryan
ross-bryan previously requested changes Feb 16, 2022
View reviewed changes
Copy link
Contributor

@ross-bryan ross-bryan left a comment

Choose a reason for hiding this comment

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

overall no major concerns. I only reviewed the code and have not spun up any RP or cluster to manually try to verify/ break with any edge cases.

pkg/api/defaults.go Show resolved Hide resolved
pkg/cluster/cluster.go Outdated Show resolved Hide resolved
pkg/cluster/deploystorage_resources.go Show resolved Hide resolved
pkg/cluster/deploystorage_resources.go Show resolved Hide resolved
pkg/cluster/deploystorage_resources.go Outdated
}

// 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

pkg/cluster/storageaccounts.go Show resolved Hide resolved
test/e2e/operator.go Outdated Show resolved Hide resolved
@25region 25region changed the title Storage account lockdown Network Storage Account Lockdown Feb 17, 2022
s-amann
s-amann previously approved these changes Feb 17, 2022
View reviewed changes
@25region 25region added the next-release To be included in the next RP release rollout label Feb 21, 2022
@bennerv bennerv removed the hold Hold label Feb 22, 2022
bennerv
bennerv requested changes Feb 23, 2022
View reviewed changes
pkg/api/defaults.go Show resolved Hide resolved
pkg/cluster/deploystorage_resources.go Show resolved Hide resolved
pkg/cluster/deploystorage_resources.go Outdated Show resolved Hide resolved
pkg/cluster/deploystorage_resources.go Outdated Show resolved Hide resolved
pkg/cluster/storageaccounts.go Show resolved Hide resolved
pkg/cluster/deploystorage_resources.go Show resolved Hide resolved
pkg/operator/controllers/storageaccounts/storageaccounts.go Outdated Show resolved Hide resolved
ross-bryan
ross-bryan previously approved these changes Feb 23, 2022
View reviewed changes
Copy link
Contributor

@ross-bryan ross-bryan left a comment

Choose a reason for hiding this comment

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

lgtm

@ross-bryan ross-bryan dismissed their stale review February 24, 2022 00:49

hmm... either really flaky e2e or something is off, going to need to dig deeper tomorrow.

@ross-bryan
Copy link
Contributor

/azp run e2e

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

bennerv
bennerv approved these changes Feb 24, 2022
View reviewed changes
Copy link
Collaborator

@bennerv bennerv left a comment

Choose a reason for hiding this comment

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

lgtm - addressed the feedback I had.

@bennerv bennerv merged commit 94c46fc into Azure:master Feb 24, 2022
@bennerv bennerv deleted the storage.lockdown.final branch February 8, 2023 17:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@bennerv bennerv bennerv approved these changes

@ross-bryan ross-bryan ross-bryan left review comments

@s-amann s-amann s-amann left review comments

@jewzaam jewzaam Awaiting requested review from jewzaam jewzaam is a code owner

@m1kola m1kola Awaiting requested review from m1kola

@mwoodson mwoodson Awaiting requested review from mwoodson

@rogbas rogbas Awaiting requested review from rogbas rogbas is a code owner

@petrkotas petrkotas Awaiting requested review from petrkotas petrkotas is a code owner

@hawkowl hawkowl Awaiting requested review from hawkowl hawkowl is a code owner

Assignees
No one assigned
Labels
next-release To be included in the next RP release rollout priority-high High priority issue or pull request size-large Size large
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
6 participants
@25region @ross-bryan @hawkowl @bennerv @s-amann @mjudeikis