search

Azure / azure-sdk-tools

Tools repository leveraged by the Azure SDK team.
MIT License
114 stars 180 forks source link

Test resource creation for developer playground should add engsys as owners #1290

Open mitchdenny opened 3 years ago

mitchdenny commented 3 years ago

We had an instance where a credential was leaked for a service principal which had access to our developer playground subscription. Whilst we were effectively able to disable the access of the service principal, we were not able to immediately rotate the credential because no-one on the EngSys team had ownership rights over the service principal.

The service principal was created by our test resource creation script when no test service principal credentials are supplied. So it creates a new one where the owner is set to the developer that ran the script. This is fine, but we should also amend this script so that the freshly minted service principal also lists the EngSys team as an administrator so that we can go in and rotate the credentials if the developer who owns it is not available.

In addition we should determine whether we want the service principal that is created locally by developers to be granted access at the subscription level or not. It may be better to grant access just to the resource group that is being created and then allow the developer to grant additional rights if it is required. This would reduce the blast radius in the event of a credential leak.

danieljurek commented 3 years ago

Agree that we should reduce blast radius of credential disclosure here. I think we can restrict access to the resource group but there may have been a reason for using the subscription.

We intended the resource creation script to also be run by external developers who want to validate proposed changes to our SDKs. To that end we'd need a way to ensure that the EngSys team identity is added as an owner to the service principal when used against one of our AAD tenants.

Another thing discussed was to find a way to keep track of service principals created by the resource scripts and delete them in certain cases (expiry, etc.).

mitchdenny commented 3 years ago

Another thing discussed was to find a way to keep track of service principals created by the resource scripts and delete them in certain cases (expiry, etc.).

AAD service principals are automatically deleted by the AAD team on a schedule, you should see deletion e-mails from time to time for any service principals that you create that become unused (I create enough for adhoc testing to see them every week or so).

Regarding the scope of permissions for the service principal, I suspect that it was set like that either because it wasn't considered a problem, or that for resources that creating the service principal with that scope made it easier for tests which also rely singleton resources not defined in the template.

benbp commented 3 years ago

@mitchdenny it seems this may not currently be supported? https://feedback.azure.com/forums/169401-azure-active-directory/suggestions/37337278-add-group-as-owner-on-azure-ad-application-and-ser

In the near term, we could have the script add individual users instead, though that has many obvious drawbacks.

mitchdenny commented 3 years ago

Wow. That is quite a limitation. I think we should have a list of folks that have owner rights then. I'd start with Wes, Mike, Scott, and myself. Although my preference would have been for anyone on the EngSys team to be able to rotate a secret.