microsoftgraph / msgraph-bicep-types

Repo contains Microsoft Graph resource types to integrate with bicep templates.
MIT License
44 stars 7 forks source link

Support setting owner for App Registration / Service Principal #134

Open xqrzd opened 5 months ago

xqrzd commented 5 months ago

Is your feature request related to a problem? Please describe. I'm trying to create an app registration and service principal, and assign app roles using the minimum permissions described here. This is done through a service connection in Azure DevOps. Based on the types I'm trying to create, the minimum permissions mentioned are

The deployment fails with forbidden. The app registration is created, but not the service principal. In order to get the service principal created I needed the max permissions:

I still ran into this issue, but I expect once that's resolved the deployment will work.

Describe the solution you'd like What I'd like is for the service account creating app registrations / service principals to only have permissions to modify applications it created, rather than everything. I think exposing owner would accomplish this, since then the service account would have permission to modify anything it created. I can't seem to test this though, as the Azure Portal seems to only allow setting users as owners, not other app registrations.

dkershaw10 commented 5 months ago

Thanks for reporting this @xqrzd. There are two problems here:

  1. This is related to #114 - once that bug is fixed, your service principal caller (creating the application) will be set as the owner. That should also enable that caller to create the SP, and also be set as an owner of the SP. That should give you the correct least privileged approach you need.
  2. Unfortunately the workaround mentioned in #114 is not available, as we did not expose owners for applications and servicePrincipals in Bicep, which was a total miss/oversight on our part. This requires a a change in the underlying service to support immutability, so it's not as simple as just adding the owners property to the Bicep type definitions. We have feature this in our backlog.

Also seems like a portal oversight that only permits users as owners.

As a workaround you should be able to use Graph REST API calls through Az PS or Az CLI to accomplish this:

POST https://graph.microsoft.com/v1.0/applications//owners/$ref Content-Type: application/json { "@odata.id":"https://graph.microsoft.com/v1.0/directoryObjects/" }

ri-we commented 2 months ago

Thanks for reporting this @xqrzd. There are two problems here:

  1. This is related to Owner of a group #114 - once that bug is fixed, your service principal caller (creating the application) will be set as the owner. That should also enable that caller to create the SP, and also be set as an owner of the SP. That should give you the correct least privileged approach you need.
  2. Unfortunately the workaround mentioned in Owner of a group #114 is not available, as we did not expose owners for applications and servicePrincipals in Bicep, which was a total miss/oversight on our part. This requires a a change in the underlying service to support immutability, so it's not as simple as just adding the owners property to the Bicep type definitions. We have feature this in our backlog.

Also seems like a portal oversight that only permits users as owners.

As a workaround you should be able to use Graph REST API calls through Az PS or Az CLI to accomplish this:

POST https://graph.microsoft.com/v1.0/applications//owners/$ref Content-Type: application/json { "@odata.id":"https://graph.microsoft.com/v1.0/directoryObjects/" }

Adding an owner to the application doesn't solve the issue, you still need Application.ReadWrite.All instead of Application.ReadWrite.OwnedBy.

You would expect Bicep to require the same least priv as the new-azadapplication command, and would also expect the same result which is by default the owner is set to the creator.

dkershaw10 commented 2 months ago

@xqrzd @ri-we sorry for the delays here. We are working on the fix here (so that this works with Application.ReadWrite.OwnedBy) and hope to have this out to you shortly. We understand how important this is for DevOps CI/CD flows (together with using a least privileged permission).

eketo-msft commented 2 months ago

@xqrzd @ri-we sorry for the delays here. We are working on the fix here (so that this works with Application.ReadWrite.OwnedBy) and hope to have this out to you shortly. We understand how important this is for DevOps CI/CD flows (together with using a least privileged permission).

The fix for this is deployed fully. You may need to manually add an owner to an EXISTING service principal to unblock your scenario, but new Apps and ServicePrincipal's will have owner automatically added when using the OwnedBy permission. If you can verify on your end that would be great.

broll commented 2 months ago

I can confirm this works better now. It still doesn't add the principal that started the bicep as owner of the app registration, but instead grants: "Microsoft Graph Bicep Extension"

As the owner.

It doesn't make anything owner to the enterprise app/app registration it creates.

eketo-msft commented 2 months ago

I can confirm this works better now. It still doesn't add the principal that started the bicep as owner of the app registration, but instead grants: "Microsoft Graph Bicep Extension"

As the owner.

It doesn't make anything owner to the enterprise app/app registration it creates.

We'll investigate this ASAP. Synthetic tests are showing the correct ownerships being added, but clearly there is something wrong if it's adding our "Microsoft Graph Bicep Extension" application.

eketo-msft commented 2 months ago

@broll, we aren't able to reproduce this in any of our environments. A few asks when you are available:

  1. Can you run again with verbose mode and make sure it is creating a brand-new app reg and service principal?
  2. From verbose mode please find and share the graph request-id's and timestamps.
  3. If you could also share the template, then we can use that to see if it repro's on our end.

Thanks in advance!

broll commented 2 months ago

Not sure how to actually get the graph client ids, they only seem to be reported when there's an error... If there's a verbose setting I'm supposed to turn on that will make them show up please let me know... This is what I tried.

Let me know

Azure Devops Pipeline

trigger: none
pool:
  vmImage: ubuntu-latest

steps:
  - checkout: self
  - task: AzureCLI@2
    displayName: Test
    name: test
    inputs:
      azureSubscription: "theServiceConnection"
      scriptType: pscore
      scriptLocation: "inlineScript"
      inlineScript: |
        az deployment sub create `
          --template-file thebicep.bicep `
          --location eastus2 `
          --subscription iamsub `
          --verbose `
          --name "principalCreationTest"
targetScope = 'subscription'

extension microsoftGraph

param azureDevOpsProjectName string = 'devopsProject'
param appRegistrationName string = 'iamatestappreg-92837'

var microsoftEntraAudience = 'api://AzureADTokenExchange'
var issuer = 'https://vstoken.dev.azure.com/e330ea89-c446-4793-9de8-85981ac5d777'
var subject = 'sc://DevopsOrg/${azureDevOpsProjectName}/${appRegistrationName}'
var description = 'AzureDevops'

resource appReg 'Microsoft.Graph/applications@v1.0' = {
  displayName: appRegistrationName
  uniqueName: appRegistrationName
}

resource sp 'Microsoft.Graph/servicePrincipals@v1.0' = {
  appId: appReg.appId
}

resource federatedCredential 'Microsoft.Graph/applications/federatedIdentityCredentials@v1.0' = {
  name: '${appReg.uniqueName}/${description}'
  audiences: [microsoftEntraAudience]
  description: description
  issuer: issuer
  subject: subject
  dependsOn: [sp]
}

@sys.description('The Service Principal Id to back the teams service connection.')
output servicePrincipalId string = sp.id

@sys.description('The App Registration Id to back the teams service connection.')
output appId string = appReg.appId
eketo-msft commented 2 months ago

Gotcha. I'll follow up on debugging info for successful requests. We return it in headers so that is likely why verbose didn't spit it out. For errors we return it in the headers AND body.

What tenant are you deploying the template in? I can try to narrow down that way.

eketo-msft commented 2 months ago

Disregard the above - I was able to isolate the requests and can confirm we see the wrong owner being set. Unfortunately, repro with your template shows the correct ownership in our Prod-test environment. I'm continuing my investigation and will reach out if I need to confirm any environment details. Thank you for your patience.

danstis commented 2 months ago

@eketo-msft I can confirm I see the same behaviour as @broll on my tenant as well: image

eketo-msft commented 2 months ago

Thank you for the repros and feedback. We were able to isolate the code issue and are reviewing our options. I'll share an ETA for a fix as soon as possible.

eketo-msft commented 1 month ago

Backend changes were made to fix this issue. Deployment is starting soon but it will take about two weeks to fully deploy and then enable the fix.

broll commented 2 weeks ago

Can confirm this is working now

eketo-msft commented 2 weeks ago

@broll, this is working consistently for you? I haven't enabled the fix yet because the deployment was delayed by the backend team. Can you share your SP ObjectId that is deploying a template and TenantId? I can dig into why it's working for you now.

I can go into more detail now about the specific issue. Calculating the claims principal, which includes the original subject service principal that initiates deployment and the Graph Bicep application, is very expensive. To mitigate the impact the backend service will serialize the claims principal and cache it to be deserialized in subsequent requests (this is also one of the reasons for the infamous Entra ID issue where it takes up to 15 minutes for permission changes to reflect on API calls -- the cache TTL is 15 minutes).

Upon investigation we discovered that the wrong GUID was being deserialized from cache for the subject ObjectId, thus if a cached principal was used then the wrong owner link would be added to the application. If the wrong owner link was added, then the resulting owner validation would fail and the client would receive an unauthorized message.

I suspect if you are seeing success then either the principal is not cached or the cache is being made invalid for some reason. The reason we didn't detect this with our synthetics is because our scenario was using invalidated cache entries, so we were always calculating a new claims principal.

Audiodude-nl commented 1 week ago

I am still seeing the behaviour as mentioned by broll, where the app gets the "Microsoft Graph Bicep Extension" set as owner. So please implement the fix.

eketo-msft commented 1 week ago

Fix is validated in our integration environment. PROD deployment will resume after the election week in the USA. ETA is Nov 15th.

Audiodude-nl commented 1 week ago

ETA for WE region ?

eketo-msft commented 16 hours ago

@Audiodude-nl I can't be positive because I'm not sure when your specific tenant will be updated, but it should be by 11/14, maybe a day or two earlier.