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.

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

Limit App permission to Storage Queue endpoint #24

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

Tbohunek
Copy link
Contributor

It is enough to use builtin RBAC role to access Queue endpoint.
It is enough to scope the role to the Storage Account.

@marcosgm
Copy link
Contributor

Are you sure this works?

Because
https://docs.microsoft.com/en-us/azure/role-based-access-control/built-in-roles#storage-queue-data-message-processor
it only has

"dataActions": [
"Microsoft.Storage/storageAccounts/queueServices/queues/messages/read",
"Microsoft.Storage/storageAccounts/queueServices/queues/messages/process/action"
],

But we also need to list storage accounts and event grid subscriptions so we're notified of new audit logs
actions = [
"Microsoft.Resources/subscriptions/resourceGroups/read",
"Microsoft.Storage/storageAccounts/read",
"Microsoft.Storage/storageAccounts/blobServices/containers/read",
"Microsoft.Storage/storageAccounts/queueServices/queues/read",
"Microsoft.EventGrid/eventSubscriptions/read",
"Microsoft.Storage/storageAccounts/listkeys/action"
]

Here are some other Built-in roles that have those missing permissions
https://docs.microsoft.com/en-us/azure/role-based-access-control/built-in-roles#eventgrid-eventsubscription-reader
https://docs.microsoft.com/en-us/azure/role-based-access-control/built-in-roles#storage-blob-data-reader

However that would involve having 3 built-in roles assigned to the service principal, instead of allowing just this one CustomRole (which would be assigned to Lacework's resource group, as per my PR#22)

@Tbohunek
Copy link
Contributor Author

@marcosgm Yes, I may be wrong. Can you explain what each permission is needed for?
Here's my reasoning:

  • EventGrid subscription works on its own, transforms blobs to messages. There's nothing to /read in it.
  • If you authenticate with proposed RBAC role, no keys are necessary. Lacework only need to read messages, not blobs.

@afiune
Copy link
Contributor

afiune commented Jul 21, 2021

@Tbohunek Thank you so much for this contribution, this is a bigger change that will require
further validation from our side, in the meantime, could you please:

  1. Rebase your PR so that it is a single signed commit, this will require you to "hard push" to your branch
  2. Resolve the conflicts you have

We will keep you posted on how the review goes. 👍🏽

@Tbohunek Tbohunek force-pushed the rbac branch 2 times, most recently from b796bf7 to 1145742 Compare July 23, 2021 10:32
Use Role Definition Name instead
@marcosgm
Copy link
Contributor

marcosgm commented Aug 9, 2021

Hi @Tbohunek . I've been working with Engineering, they're aware we can do things differently on our backend. For the time being, the integration needs the permissions we document in the Custom Role description. However if you prefer Built-in roles, you could use these two (which grant broader permissions though).

$ appID=<LW_ACTLOGREAD_APP_ID>
$ subscriptionID=$(az account show --query id --output tsv)
#Storage Account Contributor
$ az role assignment create --assignee $appID --role "17d1049b-9a84-46fb-8f53-869881c3d3ab" --scope "/subscriptions/$subscriptionID/resourceGroups/$resourcegroup"
#EventGrid EventSubscription Reader
$ az role assignment create --assignee $appID --role "2414bbcf-6497-4faf-8c65-045460748405" --scope "/subscriptions/$subscriptionID/resourceGroups/$resourcegroup"

Today, we use Azure's Log Profile export feature, which stored the logs in a folder named "insights-operational-logs". I'm working to certify our usage with Azure Monitor's Diagnostic Settings, which exports the same kind of logs in a folder named "insights-activity-logs". Inside, we'll find a folder structure that includes Subscription ID, allowing us to consolidate logs from multiple subscriptions in a single Storage account (in alignment to Microsoft's best practices for centralized logging). The blobs are created by Azure hourly, in separate folders. We leverage an Event Grid subscription that puts a Queue messages, so Lacework pulls the new logs as soon as they appear, by subscribing to the Queue and then downloading the logs only when needed.

@marcosgm
Copy link
Contributor

Closing as we consider the permissions scoped down to the resource group to be the most restrictive.
We'll re-visit removing the one or two unnecessary permissions in the custom role after we move to Diagnostic Settings (soon!)

@Tbohunek
Copy link
Contributor Author

Thanks for your efforts @marcosgm, however I think there's a little misunderstanding.
Role assignment on RG is fine indeed, but the custom role is unnecessary.

In my understanding, all your AAD App needs access to is the Storage Queue to which Event Grid Subscription feeds messages from the Storage Blob. For that, Storage Queue Data Message Processor built-in role should be enough and offer minimum permission.

Why would you need to /read the entire Event Grid Subscription and the Storage Containers? This only gives you configuration metadata which are managed by the Terraform deployment. Your app should only need data-plane access, not control-plane, and only into Queue. The rest is transparently taken care of.

Am I wrong? Thanks for your thoughts.

@afiune
Copy link
Contributor

afiune commented Mar 9, 2022

@marcosgm @Tbohunek can you folks help me understand where we are with this PR? I feel we don't need it anymore, can I close this?

@Tbohunek
Copy link
Contributor Author

@afiune I guess you just need to test it out internally.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants