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

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

Open
mitchdenny opened this issue Dec 17, 2020 · 4 comments
Assignees
Labels
Central-EngSys This issue is owned by the Engineering System team. EngSys This issue is impacting the engineering system.

Comments

@mitchdenny
Copy link
Contributor

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.

@ghost ghost added the needs-triage Workflow: This is a new issue that needs to be triaged to the appropriate team. label Dec 17, 2020
@mitchdenny mitchdenny assigned mitchdenny and unassigned mitchdenny Dec 17, 2020
@mitchdenny mitchdenny added Central-EngSys This issue is owned by the Engineering System team. EngSys This issue is impacting the engineering system. labels Dec 17, 2020
@danieljurek
Copy link
Member

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
Copy link
Contributor Author

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.

@kurtzeborn kurtzeborn removed the needs-triage Workflow: This is a new issue that needs to be triaged to the appropriate team. label Jan 4, 2021
@benbp
Copy link
Member

benbp commented Jan 27, 2021

@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
Copy link
Contributor Author

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Central-EngSys This issue is owned by the Engineering System team. EngSys This issue is impacting the engineering system.
Projects
Status: 🤔 Triage
Development

No branches or pull requests

4 participants