Issue 985 - Add support for clientApplications in conditional access policies

[benoit74]

Fix #985

I do not know yet how to add tests ; in order to really test this, we need a service principal ID, and by design those IDs are specific to a given Azure Tenant.

Should I add an additional environment variables with a service principal ID to be set to run this test ? What if this is not set, it means the test suite will fail ? Could I even add two variables with two service principal IDs to check that adding multiples items is working ok ?

Thanks in advance

[manicminer]

Thanks for opening this PR @benoit74. The acceptance tests are configured by environment variables and generally require/expect that ARM_CLIENT_ID and ARM_CLIENT_SECRET are set, although you can currently run the tests using client certificate authentication.

If you need additional resources, for example some service principals, to test this addition, you would need to include these in your test configuration - e.g. in this test for the group_member resource.

I've added some comments below, if you can take a look at these and have a go at adding the tests, then I'll be happy to re-review and we can look to get this merged.

If you need any further help, don't hesitate to ask and myself or another maintainer or community member will be happy to assist.

[manicminer]

This file cannot be changed in this repository - it must be updated upstream and then vendored into this codebase.

[benoit74]

@manicminer oups, I did not even realized I was modifying a vendored file ... sorry, I was probably too tired.
Could you please have a look / release manicminer/hamilton#235?

[benoit74]

I will have a look into tests later this week.

[benoit74]

The acceptance tests have uncovered issues and I definitely need help.

Issue 1

When the acc test tries to create an app registration and a service principal to use them in the included / excluded service principal of a conditional access policy (see clientApplicationsIncluded in internal/services/conditionalaccess/conditional_access_policy_resource_test.go for instance), there is a situation where the conditional access policy fails to be created with a message saying that the service principal does not exists. From my understanding, this is a race condition where the service principal is indeed already there but not yet propagated "everywhere" in the AD. Did you already experienced such a behavior ? Is there a good way to take care of this in the provider, or should we simply inform the users that this might happen (and everyone using Azure AD knows those strange situations ...)?

Issue 2

When moving from a conditional access policy based on client_applications to an access policy based on users, it is mandatory to explicitly set the client_applications to empty lists or the patch will fail, the client_applications will not be removed and the Graph API will complain that you cannot use client_applications and users in a same policy. The same applies the other way around (this is why I had to comment out the line 296-311 of internal/services/conditionalaccess/conditional_access_policy_resource_test.go). Once again, how to take care of this nicely in the provider (i.e. if client_applications key is absent, force the list to empty lists in the API to ensure they are removed if they were present)

Issue 3

When client_applications are used, under certain circumstances (an import ?) the provider complains that the plan is not empty. It is confused by the fact that no users is equivalent to a users with included_users set to None. Log details:

2023-03-23T13:51:59.681+0100 [DEBUG] sdk.helper_resource: Stopping providers: test_terraform_path=/usr/local/bin/terraform test_working_directory=/var/folders/cq/ry0ly6w11tz_hb08292ps2d00000gn/T/plugintest3721484953 test_name=TestAccConditionalAccessPolicy_clientApplications test_step_number=1
2023-03-23T13:51:59.682+0100 [ERROR] sdk.helper_resource: Unexpected error: test_step_number=1 test_terraform_path=/usr/local/bin/terraform test_working_directory=/var/folders/cq/ry0ly6w11tz_hb08292ps2d00000gn/T/plugintest3721484953
  error=
  | After applying this test step, the plan was not empty.
  | stdout:
  |
  |
  | Terraform used the selected providers to generate the following execution
  | plan. Resource actions are indicated with the following symbols:
  |   ~ update in-place
  |
  | Terraform will perform the following actions:
  |
  |   # azuread_conditional_access_policy.test will be updated in-place
  |   ~ resource "azuread_conditional_access_policy" "test" {
  |         id           = "c199231b-f3c8-4afc-8029-291f8c247b9f"
  |         # (2 unchanged attributes hidden)
  |
  |       ~ conditions {
  |           + sign_in_risk_levels = []
  |           + user_risk_levels    = []
  |             # (1 unchanged attribute hidden)
  |
  |           ~ applications {
  |               + excluded_applications = []
  |               + included_user_actions = []
  |                 # (1 unchanged attribute hidden)
  |             }
  |
  |           ~ client_applications {
  |               + excluded_service_principals = []
  |                 # (1 unchanged attribute hidden)
  |             }
  |
  |           - users {
  |               - excluded_groups = [] -> null
  |               - excluded_roles  = [] -> null
  |               - excluded_users  = [] -> null
  |               - included_groups = [] -> null
  |               - included_roles  = [] -> null
  |               - included_users  = [
  |                   - "None",
  |                 ] -> null
  |             }
  |         }
  |
  |       ~ grant_controls {
  |           + custom_authentication_factors = []
  |           + terms_of_use                  = []
  |             # (2 unchanged attributes hidden)
  |         }
  |     }
  |
  | Plan: 0 to add, 1 to change, 0 to destroy.
   test_name=TestAccConditionalAccessPolicy_clientApplications

I assumed that I should use the DiffSuppressFunc but failed to do so, I do not even have a clue about how to log something to check the k value (see line 483 of internal/services/conditionalaccess/conditional_access_policy_resource_test.go, this produces no log no matter the value of TF_LOG and TF_LOG_PROVIDER)

[manicminer]

Hi @benoit74, unfortunately Azure AD and MS Graph is littered with these sorts of eventual consistency gotchas. In the first instance we will try to work around this in the provider in any way we can so as not to surface this problem to the practitioner. However in this scenario I think we have already exhausted our available workarounds since the azuread_service_principal resource already retries to read and patch newly created service principals until they can be manipulated without error - so it would seem that some additional replication needs to take place in order for conditional access policies to see recently created SPs.

In this case, in order to get the tests passing, I would suggest we reference the authenticated service principal rather than creating a new one (via an azuread_client_config data source).

For the second problem, can you not add a users block with included_users = ["All"] to the new test config you're adding? You're right that a DiffSuppress func is the way to go if we need to make these two equivalent, but I'd prefer not to unless absolutely necessary.

Unfortunately, due to a licensing issue that I seem to be having with our testing tenant, which I think is an Azure problem brought about by the introduction of new Entra licenses, I am unable to test this right now and so we'll have to bump this to next week whilst we continue to iron out these kinks :)

[benoit74]

Hi @manicminer,

Very good idea to use the service principal currently authenticated!

For the second problem, yes I can add such a block and the test would be ok. My point is more about whether we should hide this complexity from the user of the provider or not. But you are right, it is probably simpler to make the users block required, and document that when using client_applications you have to set included_users = ["None"]. And the transition from client_applications to users or from users to client_applications in a given policy seems to not be feasible through the Graph API (i.e. moving from clientApplicationsIncluded to complete test data does not work), looks like you have to delete the policy and recreate it ; anyway this is really a corner case and makes very little sense from a real usage perspective.

And yes, you need to activate Microsoft Entra Workload Identities on your tenant. It is quite "easy", a person with sufficient permission just has to navigate to https://entra.microsoft.com, open the "Workload Identities" tile on the main page, and there will be a box about activating a trial of "Workload Identities Premium" on the page.

Tests are finally ok on my side. I added some extra examples, does it makes sense or is it cluttering the doc? I hope this time I fixed the doc ordering, I do not achieve to make docs-lint run on my laptop, it complains about flags missing, YAML impossible to parse, ...

[manicminer]

@benoit74 Wondered if you had any pending changes to push? If not then I'll rebase and check out the tests; I should be able to look at this in the next week. Thanks!

[benoit74]

@manicminer nope, thank you for reaching back on this. I would be glad if this could get merged soon. Do not hesitate to ask if you encounter any difficulty while testing this.

[benoit74]

@manicminer any news / problems on this issue ?

[benoit74]

@manicminer

Any chance we could release it this week?

I get that this is complex to test since it requires specific Microsoft licences, but even with the licences you can already confirm that the changes do not break anything that is working. So at the worst, it is just not working for something which is not supported as of today. And I can tell that my tests are 100% ok on my test tenant, so chances are high that it will work. Releasing this would in addition allow us to get feedback from the community on edge cases that we will never imagine on our own.

Sorry for pushing a bit hard on this, but it has already been 3 months and we are deploying more and more of these access policies and it is a pain to do by hand (we already forgot to do this once in a while).

Btw, I'm surprised that no one is complaining about this feature being not yet supported. Quite an interesting insight into the security considerations of the community 🤣

[manicminer]

Good spot on the others, I believe this one is required

Suggested change

	* `users` - (Optional) A `users` block as documented below, which specifies users, groups, and roles included in and excluded from the policy. Either `client_applications` or `users` must be specified (and not both).
	* `users` - (Required) A `users` block as documented below, which specifies users, groups, and roles included in and excluded from the policy. Either `client_applications` or `users` must be specified (and not both).

[manicminer]

Thanks for the reminders and your patience @benoit74 and apologies for the delay. We've resolved some licensing issues and the tests look good on our end too.

I've made some light changes which I don't believe affects functionality. Mainly that the users condition block stays required for now - it's proving quite difficult to reliably suppress the diff that occurs from omitting it. It also took me awhile to figure out that the API refuses service principals linked to multi-tenant apps, so I've created a persistent single-tenant app and SP on our end for testing this.

After I merged in main, I managed to break GitHub so ended up rebasing and squashing your changes to a single commit, but made sure to keep your authorship for attribution.

Thanks again for working on this, and apologies again for the delay. I believe we're now all set for additional licensed features like these going forward.

[manicminer]

Test results

[benoit74]

Great ! I Will test it in real condition as soon this is released anyway :)
Thank you for doing the final tidying pass on your own while keeping my authorship.

[github-actions]

This functionality has been released in v2.40.0 of the Terraform Provider. Please see the Terraform documentation on provider versioning or reach out if you need any assistance upgrading.

For further feature requests or bug reports with this functionality, please create a new GitHub issue following the template. Thank you!