search

hashicorp / terraform-provider-azuread

Terraform provider for Azure Active Directory
https://registry.terraform.io/providers/hashicorp/azuread/latest/docs
Mozilla Public License 2.0
432 stars 300 forks source link

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

Closed tsologub closed 1 year ago

tsologub commented 2 years ago

Community Note

Terraform (and AzureAD Provider) Version

Terraform v0.12.31
azuread v2.12.0

Affected Resource(s)

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

  2. 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.

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

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

manicminer commented 2 years ago

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 commented 2 years ago

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 commented 2 years ago

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 commented 2 years ago

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,
   "owners@odata.bind":[
      "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 owners@odata.bind in JSON.

Does this mean that Azure AD performs a GET on every entry in the owners@odata.bind 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 commented 2 years ago

@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 commented 2 years ago

@manicminer As I've mentioned in this comment https://github.com/hashicorp/terraform-provider-azuread/pull/713#issuecomment-1009979130, 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 commented 2 years ago

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 commented 2 years ago

@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 commented 2 years ago

@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 commented 2 years ago

when we have more information in the near future.

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

tsologub commented 2 years ago

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

JelleSmet-TomTom commented 2 years ago

@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 commented 2 years ago

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 commented 1 year ago

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.