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

Feature: add application filter to azuread_conditional_access_policy #1357

Open
wants to merge 61 commits into
base: main
Choose a base branch
from

Conversation

BrendanThompson
Copy link

@BrendanThompson BrendanThompson commented Apr 18, 2024

Description

This PR adds the ability to have an application filter on azuread_conditional_access_policy.

resource "azuread_conditional_access_policy" "this" {
  display_name = "access-policy"
  state        = "disabled"

  conditions {
    client_app_types    = ["other"]

    applications {
      filter {
        mode = "include"
        rule = "CustomSecurityAttribute.AttributeName -eq \"AttributeValue\""
      }
    }

    users {
      included_roles = ["Contributor"]
    }
  }
}

Change Log

Related Issue(s)

Fixes #1318

@BrendanThompson
Copy link
Author

@tombuildsstuff — don't suppose you've time to give some feedback on this PR as well?

Signed-off-by: Brendan Thompson <github@blt.is>
Signed-off-by: Brendan Thompson <github@blt.is>
Signed-off-by: Brendan Thompson <github@blt.is>
Signed-off-by: Brendan Thompson <github@blt.is>
@manicminer manicminer force-pushed the feature/application-conditional-filter branch from 84c762d to 2b43824 Compare May 8, 2024 19:36
@github-actions github-actions bot added size/S and removed size/XL labels May 8, 2024
Copy link
Contributor

@manicminer manicminer left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @TomasKunka for this addition.

This is looking great - if you could add an acceptance test case that uses a filter for an application condition then this should be good to merge. Conditional Access Policies have shown to yield odd payload parsing bugs further down the line so having coverage is necessary to give us a good shot at spotting regressions.

Thanks!

@BrendanThompson
Copy link
Author

@manicminer thanks for reviewing this, I will try and get an acceptance test in today 😃

@manicminer
Copy link
Contributor

manicminer commented May 9, 2024

@BrendanThompson Great thanks, and sorry for the mis-tag! 😊

@BrendanThompson
Copy link
Author

@manicminer — sorry it's taken me a few days to get back to you. I started writing the tests then I realised that we will actually require some attributes to be set on wherever the Acceptance Tests are run, otherwise the tests will fail. There is currently also no mechanism to deploy said attributes with Terraform ☹️.

https://learn.microsoft.com/en-us/entra/fundamentals/custom-security-attributes-overview

The above link are the attributes we need in-place. How would you like to proceed on this, if possible were you able to add an attribute and I can test with that. Then perhaps the next thing is to work on integrating the attribute creation into the provider? (Happy to work in this)

@manicminer
Copy link
Contributor

@BrendanThompson All good, appreciate you looking at this. Unfortunately these are not currently supported by the provider. I don't believe they are supported by the current SDK either so this would have to be added throughout in order to integrate custom security attributes. Is it not maybe possible to build a rule for testing without using custom attributes? Even if it's essentially a no-op that would be fine.

@BrendanThompson
Copy link
Author

I cannot think of any way to test it properly without the custom attributes being there to be honest. And testing it without an attribute would yield odd results due to the API. We could add a notice on the resource itself, like a warning around it. Buyer beware until the security attributes are available. What are your thoughts?

@manicminer
Copy link
Contributor

I would also love to ship this but unless we are able to demonstrate some level of testing, we unfortunately won't be able to add a property or block to a resource without it, which I guess means this will have to wait until we have a resource to support custom security attributes.

@BrendanThompson
Copy link
Author

Hey @manicminer,

I have opened a PR in Hamilton (manicminer/hamilton#281) to add support for both attribute sets and custom security attribute definitions once that has gone through I will add support for them in the provider and update the tests here 😃

@manicminer
Copy link
Contributor

@BrendanThompson The hamilton version has been updated in the provider, hopefully this unblocks your work on this 🙂

…`app_role`, `oauth2_permission_scope`, `identifier_uris`, `optional_claims`, and `required_resource_access`
…om template

The `/instantiate` operation can return a 404 whlst also processing the
request completely normally. This leads to orphaned objects in the
directory, and a resource that cannot successfully Create.

Work around this by polling for the application and service principal
that you'd expect to see created out-of-band, whenever a 404 is
received. Also set a temporary `displayName` for the application, as
this is the only means we have to identify the resulting object is the
one we are looking for.

Unfortunately this means that this workaround cannot be implemented for
the `azuread_application_from_template` resource, since that resource
intentionally avoids changing the implicitly created
`user_impersonation` scope - this will get created with nonsensical
display names / descriptions in the consent flow. Since the whole point
of the standalone resource is to inherit scopes from the template, this
makes it infeasible to add this workaround there.
@jwelker9
Copy link

I'm really hoping to use I have nothing valuable to add... but I just wanted to cheer you guys on as I'm hoping to leverage this appFilter as well lol thanks for your work on it!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment