Azure / deployment-stacks

Contains Deployment Stacks CLI scripts and releases
MIT License
87 stars 6 forks source link

Contributor role has access to deployment stacks #106

Closed slavizh closed 2 months ago

slavizh commented 1 year ago

One of the things I have given feedback before is that for Contributor role to have access to Deployment Stacks resources. It will be way better experience if Contributor role does not have access to Stacks and you will additional role like Deployment Stacks Contributor that will give you access. For example this was done for Azure Policy when it was introduced where if you have Contributor role you do not have access to create and manage policies and you need a separate role to do that. This will help in giving higher permissions to engineers like Contributor but not allow them to manage the stack and thus overcome any restrictions place by the stacks configuration on resources.

azcloudfarmer commented 1 year ago

Hi Stanislav, a few questions below:

  1. Does the ability to store the stack at a higher scope than that of the deployment scope, help with this problem at all?
  2. Permission model for deployments. With the suggested approach, less people would have access to Deployment Stacks.
slavizh commented 1 year ago

Hi Angel,

  1. It helps in certain situations. Some customers do not have management groups setup and for that scenario it is not solving the problem. The other is that people should be able to view the deployment stack but not edit it and if you give people permissions at subscription scope but not at management group scope they will not see it
  2. I think that is the idea. Overall Deployment Stacks has security context as a feature. Due to that it should not be possible to overcome that by having Contributor access, you will need special role otherwise if you have contributor you could just easily modify the stack, add yourself to the exception list and start changing stuff.
majastrz commented 1 year ago

This was the reason we excluded Microsoft.Blueprint/blueprintAssignments/write and Microsoft.Blueprint/blueprintAssignments/delete actions from the Contributor role. We may need to do the same for Stacks.

humanascode commented 1 year ago

I agree with @slavizh. Giving the contributor role the ability to modify the stack could cause customers to start creating custom roles such as "Contributor Without Stack Access" which might push them away from the solution.

alex-frankel commented 1 year ago

I appreciate the need for this, but the concern is that this really limits who can get the lifecycle benefits of stacks. If I am a "normal" low permissions bicep customer, I might want to be able to destroy an environment or delete resources I don't need w/o necessarily needing the Deny Assignment behavior. The permissions should be decoupled such that I can get as many benefits of stacks as possible (even if I shouldn't be allowed to mess with deny assignments).

slavizh commented 9 months ago

@azcloudfarmer @alex-frankel @majastrz I did additional testing on this scenario. I have two users - user1 and user2.user1 has owner permissions and creates the deployment stack with Deny write and delete. user2 has contributor permissions. The stack is deployed at subscription scope. I have tried to delete the stack with user2 but got the following error:

{ "code": "DeploymentStackDeleteResourcesFailed", "message": "One or more resources could not be deleted. Correlation id: '31babf49-53df-4970-ab46-67ddc7c1b700'.", "details": [ { "code": "DeploymentStackDeleteResourcesFailed", "message": "An unknown error occurred while trying to delete resources. These resources are still present in the stack but can be deleted manually." } ] }

It takes some time for this error to appear and it is not stating what is the exact issue. Additionally when you try to delete the stack and it fails it puts the stack in failed state.

I have also tried to re-deploy the stack with user2 and it started deployment but it was failing on every resource within the stack with deny to write. Stack also turns to failed state.

I have also tried to re-deploy the stack with providing user2 as excluded principal. Deployment started but resources could not be updated however the stack had user2 as excluded principal applied. Stack turns to failed state.

So the stack is not protected and anyone with Contributor permissions can modify it but it cannot modify the resources. Although this is good somehow the experience is not the best:

I wish the above can be fixed in some way that if you try to modify/delete stack you are stopped from any actions if you are not part of excluded principals or the account which deployed the stack initially.

slavizh commented 9 months ago

@azcloudfarmer @alex-frankel @majastrz after all these changes I had to re-deploy the stack with user1 as I couldn't delete the stack with any of the users. After re-deploy and that user2 was added to excluded principal lists fully I still couldn't delete the stack with user2. Managed it to delete it at the end with user1.

azcloudfarmer commented 9 months ago

Hi @slavizh - thanks for the details. We plan to add checks to our validation (preflight) to catch this error sooner and deliver a better message. Otherwise, all of the behavior looks to be as expected.

Would these changes help solve your issue?

slavizh commented 9 months ago

Yes if it prevents these issues to happen in first place.

azcloudfarmer commented 8 months ago

Adding work item reference: https://msazure.visualstudio.com/One/_workitems/edit/25826207

snarkywolverine commented 7 months ago

Now tracked internally by https://msazure.visualstudio.com/One/_workitems/edit/25588407

snarkywolverine commented 2 months ago

We're planning to solve this problem with #163 - which will take effect over the next couple of weeks for all API versions.

We have essentially decoupled the deny assignment permissions from stack read/write. Contributors will be able to modify stacks without deny assignments (and won't be able to create a stack with deny assignments), but can use stacks without deny assignment configuration.

We're also creating Deployment Stack Contributor and Deployment Stack Owner roles - the latter of which can create/modify stacks with Deny Assignments. We hope to roll those out next week. #164 is tracking that part.

Given those two tasks, is there any objection to closing this issue? Or are there still scenarios that need addressing?

slavizh commented 2 months ago

@snarkywolverine no, I think this could solve the problem. If I see any issues with the new behavior we can always have a new discussion.