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

DirectoryObjects.BaseClient.Get() 403 during assigning the owners for azuread_application #703

Closed
tsologub opened this issue Dec 21, 2021 · 14 comments · Fixed by #713 or #1214
Closed

Comments

@tsologub
Copy link

Community Note

  • Please vote on this issue by adding a 👍 reaction to the original issue to help the community and maintainers prioritise this request
  • Please do not leave "+1" or "me too" comments, they generate extra noise for issue followers and do not help prioritise the request
  • If you are interested in working on this issue or have submitted a pull request, please leave a comment

Terraform (and AzureAD Provider) Version

Terraform v0.12.31
azuread v2.12.0

Affected Resource(s)

  • azuread_application

Terraform Configuration Files

provider "azuread" {
  client_id = "id"
  client_secret = "secret"
  tenant_id = "id"
}

provider "azurerm" {
  subscription_id = "id"
  client_id = "id"
  client_secret = "secret"
  tenant_id = "id"
  features {}
  alias = "provider_azurerm"
  skip_provider_registration = true
}

provider "random" {
  alias = "provider_random"
}

provider "time" {
  alias = "provider_time"
}

locals {
  password_create_wait = "3s"
  role_assignment_wait = "3s"
}

resource "azuread_application" "app_tas-graph" {
  display_name               = "tas-graph"
  identifier_uris            = []
  sign_in_audience           = "AzureADMyOrg"
  owners                     = [
    "object_id_from_authenticated_service_principal", 
    "human_object_id_who_is_the_owner_of_authenticated_service_principal", 
    "another_human_object_id_who_is_the_owner_of_authenticated_service_principal", 
    "another_human_object_id_who_is_the_owner_of_authenticated_service_principal", 
    "another_human_object_id_who_is_the_owner_of_authenticated_service_principal"
  ]
  prevent_duplicate_names    = true
  web {
    redirect_uris            = ["https://tas-graph/"]
  }
}

resource "azuread_service_principal" "spn_tas-graph" {
  application_id               = azuread_application.app_tas-graph.application_id
  app_role_assignment_required = true
  tags = [ "some", "tags" ]
  owners                     = [
    "object_id_from_authenticated_service_principal", 
    "human_object_id_who_is_the_owner_of_authenticated_service_principal", 
    "another_human_object_id_who_is_the_owner_of_authenticated_service_principal", 
    "another_human_object_id_who_is_the_owner_of_authenticated_service_principal", 
    "another_human_object_id_who_is_the_owner_of_authenticated_service_principal"
  ]
}


resource "azuread_service_principal_password" "tas-graph" {
  service_principal_id  = azuread_service_principal.spn_tas-graph.id
}


resource "azuread_service_principal_password" "tas-graph_v1" {
  depends_on            = [azuread_service_principal_password.tas-graph]
  service_principal_id  = azuread_service_principal.spn_tas-graph.id
}

resource "azuread_service_principal_password" "tas-graph_v2" {
  depends_on            = [azuread_service_principal_password.tas-graph_v1]
  service_principal_id  = azuread_service_principal.spn_tas-graph.id
}

resource "time_sleep" "tas-graph_v2_create" {
  provider = time.provider_time
  depends_on = [ azuread_service_principal_password.tas-graph_v2 ]
  create_duration = local.password_create_wait
}

resource "random_uuid" "tas-graph_role_0_Contributor" {
  provider             = random.provider_random
}

resource "azurerm_role_assignment" "tas-graph_role_0_Contributor" {
  provider             = azurerm.provider_azurerm
  name                 = random_uuid.tas-graph_role_0_Contributor.result
  scope                = "/subscriptions/cbba8e52-d657-499e-a8ac-eafbc751129b"
  role_definition_name = "Contributor"
  principal_id         = azuread_service_principal.spn_tas-graph.object_id
  skip_service_principal_aad_check = true
}

resource "time_sleep" "tas-graph_role_0_Contributor" {
  provider = time.provider_time
  depends_on = [ azurerm_role_assignment.tas-graph_role_0_Contributor ]
  create_duration = local.role_assignment_wait
}

Debug Output

https://gist.github.com/tsologub/9d28a1f8100948ea8903d1e15715e405

Expected Behavior

According to this note, I expect the Application.ReadWrite.OwnedBy should be sufficient to populate the owners.

The authenticated service principal has owners. The new application will receive as owners the authenticated service principal plus its owners.

Actual Behavior

"code":"Authorization_RequestDenied","message":"Insufficient privileges to complete the operation."

Steps to Reproduce

  1. terraform apply

Important Factoids

  1. I have configured the following permissions.

Screenshot 2021-12-21 at 11 00 06

  1. Observing the GET requests from the debug output, I can see that the .../directoryObjects/... endpoint is being used.
    I don't understand why for GET https://graph.microsoft.com/v1.0/<tenant_id>/directoryObjects/<object_id_from_authenticated_service_principal> returns the 200. And for GET /v1.0/<tenant-id>/directoryObjects/<human_object_id_who_is_the_owner_of_authenticated_service_principal> returns 403.

  2. I have double-checked all the object IDs. They are correct.

  3. Everything is working if I include only the authenticated service principal in owners.

@tsologub tsologub changed the title DirectoryObjects.BaseClient.Get() 403 during assigning the owners for service principal DirectoryObjects.BaseClient.Get() 403 during assigning the owners for azuread_application Dec 21, 2021
@manicminer
Copy link
Contributor

Hi @tsologub, thanks for the detailed report. This possibly requires User.Read.All in order to retrieve the other user owner, whereas retrieving the object for oneself is implicitly allowed. Does this also happen when you try to add another service principal as an owner? Does the error go away if you grant User.Read.All for the calling principal? I'll do some testing to try and reproduce this.

I'm planning some work to improve the enumeration of members/owners across the provider, as whilst our current approach was initially robust, there have been a number of API changes which cause incidental issues.

@tsologub
Copy link
Author

Yes, I have tested and can confirm. Adding User.Read.All solves the issue.

Does the error go away if you grant User.Read.All for the calling principal?

Yes, the error goes away.

Does this also happen when you try to add another service principal as an owner?

Funny thing, but no.

Screenshot 2021-12-21 at 17 08 49

This set of permissions is sufficient to set as an owner any service principal. But if I want to set a user, as owner... the good old 403 appears.

@manicminer
Copy link
Contributor

Thanks @tsologub, that correlates with my expectations. With a default configuration, service principals are readable by anyone. I'll factor this in and see what I can come up with. We need to improve our testing with minimal permission grants to try and catch these earlier, but I believe we'll be able to resolve this.

@tsologub
Copy link
Author

tsologub commented Jan 7, 2022

I was trying to implement this one on my own and dug deeper into this. Have some findings that I'd like to share.
I have these permissions for my tests:
Screenshot 2022-01-07 at 07 46 02


First, I found the code line which causes this behavior. Of course, when we make a GET on .../directoryObject/<user_id>, my permissions aren't sufficient. We use that line only to check the "existence", so for testing purposes, I decided to omit that and came up with this.

During terraform apply, one of the POST requests looks like following:

2022/01/07 07:20:37 [DEBUG] ============================ Begin AzureAD Request ============================
Request ID: 89ebb2af-7051-54e4-69d4-e919d5ed352e

POST /beta/<tenant_id>/applications HTTP/1.1
Host: graph.microsoft.com
User-Agent: HashiCorp Terraform/1.1.2 (+https://www.terraform.io) Terraform Plugin SDK/2.8.0 terraform-provider-azuread/dev Hamilton (Go-http-client/1.1) pid-222c6c49-1b0a-5959-a213-6608f9eb8820
Content-Length: 1050
Accept: application/json; charset=utf-8; IEEE754Compatible=false; odata.metadata=full
Content-Type: application/json; charset=utf-8
Odata-Maxversion: 4.0
Odata-Version: 4.0
Accept-Encoding: gzip

{
   "groupMembershipClaims":null,
   "[email protected]":[
      "https://graph.microsoft.com/v1.0/c7ff86b7-6464-43b5-8da5-b6209a6800c1/directoryObjects/<object_id_from_authenticated_service_principal>",
      "https://graph.microsoft.com/v1.0/c7ff86b7-6464-43b5-8da5-b6209a6800c1/directoryObjects/<object_id_user>"
   ],
   "api":{
      "acceptMappedClaims":false,
      "knownClientApplications":[
      ],
      "oauth2PermissionScopes":[
      ],
      "requestedAccessTokenVersion":1
   },
   "appRoles":[
   ],
   "displayName":"TERRAFORM_UPDATE_6c3df02c-bf59-9877-99f6-6a976ee3d360",
   "identifierUris":[
   ],
   "info":{
      "marketingUrl":"",
      "privacyStatementUrl":"",
      "supportUrl":"",
      "termsOfServiceUrl":""
   },
   "isDeviceOnlyAuthSupported":false,
   "isFallbackPublicClient":false,
   "oauth2RequirePostResponse":false,
   "optionalClaims":{
   },
   "publicClient":{
      "redirectUris":[
      ]
   },
   "requiredResourceAccess":[
   ],
   "signInAudience":"AzureADMyOrg",
   "spa":{
      "redirectUris":[
      ]
   },
   "tags":[
   ],
   "web":{
     ...
   }
}
============================= End AzureAD Request =============================: timestamp=2022-01-07T07:20:37.523+0100
2022-01-07T07:20:37.596+0100 [INFO]  provider.terraform-provider-azuread: 2022/01/07 07:20:37 [DEBUG] ============================ Begin AzureAD Response ===========================
POST https://graph.microsoft.com/beta/<tenant_id>/applications
Request ID: 89ebb2af-7051-54e4-69d4-e919d5ed352e

HTTP/1.1 403 Forbidden
Transfer-Encoding: chunked
Cache-Control: no-cache
Client-Request-Id: b4468860-4614-4369-8038-47061de98a23
Content-Type: application/json
Date: Fri, 07 Jan 2022 06:20:37 GMT
Deprecation: Mon, 05 Apr 2021 23:59:59 GMT
Link: <https://developer.microsoft-tst.com/en-us/graph/changes?$filterby=beta,PrivatePreview:Restricted_AU_Properties&from=2021-04-01&to=2021-05-01>;rel="deprecation";type="text/html"
Link: <https://developer.microsoft-tst.com/en-us/graph/changes?$filterby=beta,PrivatePreview:HasExtendedValue&from=2021-10-01&to=2021-11-01>;rel="deprecation";type="text/html"
Link: <https://developer.microsoft-tst.com/en-us/graph/changes?$filterby=beta,PrivatePreview:RequestSignatureVerification&from=2021-10-01&to=2021-11-01>;rel="deprecation";type="text/html"
Link: <https://developer.microsoft-tst.com/en-us/graph/changes?$filterby=beta,PrivatePreview:RequestSignatureVerification&from=2021-10-01&to=2021-11-01>;rel="deprecation";type="text/html"
Link: <https://developer.microsoft-tst.com/en-us/graph/changes?$filterby=beta,PrivatePreview:RequestSignatureVerification&from=2021-10-01&to=2021-11-01>;rel="deprecation";type="text/html"
Request-Id: b4468860-4614-4369-8038-47061de98a23
Strict-Transport-Security: max-age=31536000
Sunset: Sun, 09 Jan 2022 23:59:59 GMT
Vary: Accept-Encoding
X-Ms-Ags-Diagnostic: {"ServerInfo":{"DataCenter":"West Europe","Slice":"E","Ring":"5","ScaleUnit":"002","RoleInstance":"AM2PEPF0000B26F"}}
X-Ms-Resource-Unit: 1

10a
{"error":{"code":"Authorization_RequestDenied","message":"Insufficient privileges to complete the operation.","innerError":{"date":"2022-01-07T06:20:37","request-id":"b4468860-4614-4369-8038-47061de98a23","client-request-id":"b4468860-4614-4369-8038-47061de98a23"}}}
0


============================= End AzureAD Response ============================: timestamp=2022-01-07T07:20:37.596+0100

The provisioning succeeds if I remove the object_id_user from [email protected] in JSON.

Does this mean that Azure AD performs a GET on every entry in the [email protected] behind the scenes? If yes, I am afraid we can't do much from the terraform-provider-azuread perspective.

p.s. Do you think we can update hamilton here to v1.0? (I would be glad to work on that)

@manicminer
Copy link
Contributor

@tsologub I believe #713 should resolve this. We don't actually need to call the /directoryObjects endpoint any more, at least for applications or service principals. Managing owners for groups will however continue to require permission to retrieve each owner principal.

@tsologub
Copy link
Author

tsologub commented Jan 14, 2022

@manicminer As I've mentioned in this comment #713 (comment), the #713 doesn't solve this issue. I didn't fully get why the PR was merged. 😮 Could you reproduce it on your side? What is the solution?

@manicminer manicminer reopened this Jan 14, 2022
@manicminer
Copy link
Contributor

Hi @tsologub, the PR made sense to merge even if it did not necessarily fix the issue, although I intended to not close the issue - so I've reopened it. Sorry about that :)

@tsologub
Copy link
Author

@manicminer I have noticed that it works if one populates the owners via a separate AddOwners request.
i.e., First, CreateApplication without any owners (only the calling service principals will be auto-set to owners list). Then, AddOwners with a list of service principals/users/etc. <-- in this case, only the Application.ReadWrite.Ownedby is sufficient.

Mapping my words onto code - https://github.com/tsologub/terraform-provider-azuread/pull/1/files In that example, all the owners (except for calling service principal) are added via the AddOwners request.

@manicminer
Copy link
Contributor

@tsologub Thanks for the information on the workaround. I have noticed several times when creating applications with no specified owners, that sometimes the app will be auto-assigned the caller as owner but other times will be created with no owners at all. Given this, and that it seems the API behavior is changing WRT ownership, I'm hesitant to change the way we assign owners at this time, but will revisit when we have more information in the near future.

@JelleSmet-TomTom
Copy link

JelleSmet-TomTom commented Mar 15, 2022

when we have more information in the near future.

any updates or next steps about this? near future is a bit opaque.

@tsologub
Copy link
Author

To get us unblocked, we went with including in owners only the object_id_from_authenticated_service_principal. No users or groups.

@JelleSmet-TomTom
Copy link

JelleSmet-TomTom commented Mar 15, 2022

@tsologub tnx for the suggestion. The work-around on my end is to perform 2 consecutive TF runs. One with object_id_from_authenticated_service_principal set and a second run with object_id_from_authenticated_service_principal set + additional users :/ this seems to work ...

The Service Principal used had following rights: Application.ReadWrite.OwnedBy

@JelleSmet-TomTom
Copy link

JelleSmet-TomTom commented Mar 16, 2022

The provider documentation mentions Application.ReadWrite.All is advised to be used over Application.ReadWrite.OwnedBy but in my experience this doesn't make a difference so the provider does not behave as documented.

@manicminer
Copy link
Contributor

Just to follow up on this issue; as I mentioned previously we're hesitant to change the behavior of the azuread_application resource with respect to owners, since the current behavior is well established and even the smallest behavioral tweaks risk breaking existing configurations. Additionally, we have to maneuver within the constraints of Terraform's architecture, both current and past, to avoid breaking users across Terraform versions. When you factor in the characteristics of the API, which vary depending on the permissions and authentication scenario, there's a very small window for movement in the current application resource.

With that in mind, we're creating a new set of resources that approach application management somewhat differently. These will be smaller in scope, allowing practitioners to select the functionality they want and obtain additional functionality not afforded by the existing resource. Accordingly I've linked this issue to #1214 and it will be closed as resolved shortly.

Our recommendation will be to consider adopting the soon-to-be-introduced azuread_application_registration and azuread_application_owner resources. These resources yield to the API in assigning default ownership, whilst also allowing precise addition of more owners with minimal permissions needed as afforded to you by the API. Thanks for all the feedback so far.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Oct 19, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.