hashicorp / terraform-provider-azuread

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

Is it possible to be authoritative on approle-assignments for a particular approle. #679

Open gtmtech opened 2 years ago

gtmtech commented 2 years ago

Closely related to https://github.com/hashicorp/terraform-provider-azuread/issues/306, I used to use the virtual azuread_application_app_role resource to create app-roles on eapps that were given to me by another team to manage. I had no ability to create eapps or manage the eapp resource, only approles on the eapps.

A requirement has now come in that the tooling I work on is authoritative for all approle-assignments on an eapp. That is: if an approle-assignment has been added outside of the tooling by some other means, it is required to remove it, and have only one source-of-truth (in this case terraform with terraform-provider-azuread, if it is possible).

Is this possible using the current resource list. With azuread_application_app_role being removed, it would seem I have to:

  1. Import an existing eapp as an azuread_application first.
  2. Set ignore_changes on most things as it is managed by another team
  3. Create several approle blocks for my approles
  4. But there is no mechanism to set authoritatively all the approle_assignments that should exist. I can only add or remove over time ones that terraform knows about, but I cannot delete any that are not in the terraform state file.

Is this a correct interpretation of the current state of the azuread provider?

Thanks in advance.

manicminer commented 2 years ago

Hi @gtmtech, I believe it's possible to meet your requirements with the provider. For some background, we did indeed remove the azuread_application_app_role resource in order to resolve a conflict with, and improve handling of, the corresponding app_role block in the azuread_application resource.

As of v2.0 of the provider, the app_role block is no longer Computed, which means these are now managed authoritatively. Any app roles present for an application that are not in your configuration will be removed by Terraform. Many other properties are also no longer Computed for similar reasons, this means that Terraform will attempt to manage most properties in the same way, i.e. removing or unsetting values that are not defined in your configuration. This is more in line with general expected behavior.

To stop managing a property, you will need to add it to the ignore_changes list via the lifecycle meta block. For resources that are pre-created out of band, such as your application being provisioned by another team, you would need to import them into Terraform in order to manage them.

gtmtech commented 2 years ago

THanks @manicminer - it sounds like the apps are authoritative for app_roles, but perhaps the app_roles arent yet authoritative for approle_assignments (which we'd also need). (Would want to ensure nobody could be a member of an approle outside terraform)

I will POC and confirm and get back to you.

manicminer commented 2 years ago

@gtmtech Ah right, sorry I misread your question. Your interpretation is correct, at the moment we don't have a way to authoritatively configure app role assignments for a specific app role, or service principal. At face value I don't see a reason why we could not implement such a resource, subject to feasibility testing.

gtmtech commented 2 years ago

This would be great - would you take a pull request on this?

I was thinking to do something similar to the google provider (GCP) where they make a distinction between authoritative and non-authoritative policies.

I am guessing I am thinking to create a new azuread_application_app_role_assignments (plural) resource with a List parameter, which would then detect drift across all member assignments, deleting any users not in the passed List.

The only unknown is where an engineer has then used both azuread_application_app_role_assignments AND multiple separate azuread_application_app_role_assignment resources. I think the documentation would just have to make clear its one or the other (I think there is precedent for this in aws security group rules?)

Let me know what you think @manicminer - thanks!

manicminer commented 2 years ago

@gtmtech Yes, a PR would be great!

Your proposal sounds good, although I wonder if it might be best to add this as an attribute to the azuread_service_principal resource, since that's the entity for which it would be authoritative?

In terms of conflict between this and the app_role_assignment resource, yes we'd just document this caveat and advise users to not use both resources for the same service principal.

gtmtech commented 2 years ago

Hi @manicminer , I have started work on this, however have noticed:

In the process, I realised that in order for my approles to appear in the myapplications.microsoft.com portal, they have to be set on the azuread_service_principal object, and not the azuread_application object. Also it requires slightly different permissions to set approles on the service_principal as opposed to the enterprise application, and it is possible to be in an organisation with only these grants.

But within the exisiting documented azuread terraform provider, azuread_service_principal seems to have no support for adding app_role{} blocks, you can only add app_role {} blocks to the azuread_application. This seems an omission to me as it is possible to have different approles on the service principal than on the enterprise application itself. Looking at our applications for example, we have only msiam approles on the enterprise-application object, but many approles on the service_principal which correspond with aws iam roles for azure to aws federation.

Shall I add in app_role {} block support for the service_principal object, in the same way as you have it for application object?

I need to do this before I can get the assignments stuff working.

manicminer commented 2 years ago

Hi @gtmtech, that's fantastic, thanks for working on this.

The app roles for a service principal are actually inherited from the linked application. The application defines the roles, and service principals that are linked to that application can declare role assignments on behalf of users, groups or SPs in that tenant. Due to the multi-tenant architecture of applications, service principals in different tenants can be linked to the same application and this is why assignments happen from the perspective of the service principal (and not the application).

The app_roles and oauth2_permission_scopes attributes exported by the azuread_service_principal resource are actually populated from the linked application (by the API) - this allows SPs to see roles declared from the application which may be homed in a remote tenant. In a single tenant scenario, this looks like duplication but the distinction is still there nonetheless.

Hope that makes sense :)

NB: an "enterprise application" is the service principal, and not the app registration (not confusing at all I know :p )

NB2: whilst "enterprise applications" (i.e. service principals) can exist without being linked to an app registration, these are considered legacy by Azure and aren't supported by the API

gtmtech commented 2 years ago

Hi @manicminer , very useful info thanks!

So on our azure setup, when I query approles on one of our service principals I get this:

"appRoles": [
        {
          "allowedMemberTypes": [
            "User"
          ],
          "description": "msiam_access",
          "displayName": "msiam_access",
          "id": "xxxxxxxx-xxxx-xxxx-xxxx-xxxxxxxxxxxx",
          "isEnabled": true,
          "origin": "Application",
          "value": null
        },
        {
          "allowedMemberTypes": [
            "User"
          ],
          "description": "123456789012-my-iam-role,WAAD",
          "displayName": "my-iam-role",
          "id": "xxxxxxxx-xxxx-xxxx-xxxx-xxxxxxxxxxxx",
          "isEnabled": true,
          "origin": "ServicePrincipal",
          "value": "arn:aws:iam::123456789012:role/my-iam-role,arn:aws:iam::123456789012:saml-provider/WAAD"
        }...

Note the first one with origin=Application, but the second has origin=ServicePrincipal. Indeed if I query the associated application itself, I only see appRoles which in this list have an origin of Application. Therefore I believe you can add AppRoles to a Service Principal as well as an Application.

Indeed, after terraform importing both the azuread_service_principal and the associated azuread_application objects, I see different appRoles on each, though the azuread_application appRoles are a subset of the azuread_service_principal appRoles, therefore I think you are correct about those being inherited appRoles from the application itself (something I did not realise).

See my terraform import of both:

# the application has just one msiam approle

resource "azuread_application" "myapp" {
    app_role_ids                   = {}
    application_id                 = "xxxxxxxx-xxxx-xxxx-xxxx-xxxxxxxxxxxx"
    device_only_auth_enabled       = false
    disabled_by_microsoft          = "<nil>"
    display_name                   = "myapp"
   ...

    app_role {
        allowed_member_types = [
            "User",
        ]
        description          = "msiam_access"
        display_name         = "msiam_access"
        enabled              = true
        id                   = "xxxxxxxx-xxxx-xxxx-xxxx-xxxxxxxxxxxx"
    }

    feature_tags {
    ...
}

# but the related service principal has more than one appRole, but includes the inherited one.
resource "azuread_service_principal" "myapp" {
    account_enabled               = true
    alternative_names             = []
    app_role_assignment_required  = true
    app_role_ids                  = {
        "arn:aws:iam::123456789012:role/my-iam-role,arn:aws:iam::123456789012:saml-provider/WAAD"          = "xxxxxxxx-xxxx-xxxx-xxxx-xxxxxxxxxxxx"
        ...
    },
    app_roles = [
app_roles                     = [
        {
            allowed_member_types = [
                "User",
            ]
            description          = "msiam_access"
            display_name         = "msiam_access"
            enabled              = true
            id                   = "xxxxxxxx-xxxx-xxxx-xxxx-xxxxxxxxxxxx"
            value                = ""
        },
        {
            allowed_member_types = [
                "User",
            ]
            description          = "123456789012-my-iam-role,WAAD"
            display_name         = "my-iam-role"
            enabled              = true
            id                   = "xxxxxxxx-xxxx-xxxx-xxxx-xxxxxxxxxxxx"
            value                = "arn:aws:iam::123456789012:role/my-iam-role,arn:aws:iam::123456789012:saml-provider/WAAD"
        },
        ...

The docs are not clear on this (https://docs.microsoft.com/en-us/graph/api/resources/serviceprincipal?view=graph-rest-1.0) make it sound like appRoles are inherited for servicePrincipals, but they are both inherited and additive.

All this leads me to still believe we need to support appRoles on the azuread_service_principal object as separate to those on the azuread_application object.

I will check if we have done something wrong that is allowed by chance rather than by design, as the docs dont mention this explicitly.

Thanks for clearing up some of the language in the GUI it was useful to know.

gtmtech commented 2 years ago

@manicminer I checked with my Azure guy, and he pointed me to this documentation from microsoft whereby they advise you to specifically patch the servicePrincipals appRoles with extra appRoles to make aws sso work. (This is what we are doing).

So it seems their API docs dont fully explain that you can have extra appRoles on service_principals, but their other docs say this is how you get stuff working:

https://docs.microsoft.com/en-us/azure/active-directory/saas-apps/aws-multi-accounts-tutorial

(near the bottom of this very long tutorial - search for f. From the list of service principals, get the one you need to modify)

manicminer commented 2 years ago

@gtmtech Very interesting! I have heard about app roles being configured for service principals before but I believed this to be legacy behavior. I have tried this before with MS Graph but with my test configurations it simply ignored any app roles I attempted to add to the service principal manifest - it looks like to enable this for the service principal, the application must first export an app role named msiam_access?

In theory we'd be happy to support this given the API allows it per the docs you linked. In practice it's difficult to present a good UX around this because the same property has values sourced from multiple places, which isn't very declarative from the perspective of a single resource in Terraform.

gtmtech commented 2 years ago

Great, I'll work on something that works in our usecase and get back to you with a PR for comments 👍

manicminer commented 2 years ago

Hi @gtmtech, just wondering if you'd perhaps had any time to work on this? No worries if not, just checking to avoid duplication of effort :)

gtmtech commented 2 years ago

Hi @manicminer - I've gotten round to doing some work on this -

I have one file to complete before the code is ready, probably need to add some tests too.

These are my ideas - https://github.com/gtmtechltd/terraform-provider-azuread/tree/gm-add-authoritative-approles-and-assignments

Let me know what you think so far - I've not yet tested for bugs, so use with caution

gtmtech commented 2 years ago

To document the above a bit:

In my PR, you can now add sp_app_role blocks on a service principal, which add approles to the service-principal object only, whilst keeping the app_roles computed attribute for backwards compatability

Then I've created an app_role_assignments plural resource whose job it is to authoritatively declare the assignments (and the only assignments) on a App or SP. I'm just finishing that one off as it has to reconcile the list brought back from the hamilton library and fix the deltas. (I think I need to introduce an Update method, because IMO the resource shouldnt delete and re-add, as is the strategy in the non-authoritative azuread_app_role_assignment resource)

gtmtech commented 2 years ago

Am still working on this, sorry about the slow progress @manicminer - still awaiting some permissions for enterprise apps would you believe. I hope to have it done within the next month

manicminer commented 2 years ago

All good, thanks for the update @gtmtech 👍

gtmtech commented 2 years ago

Hi @manicminer

I believe I've finished the code for this (it's currently here in case you want to see it https://github.com/gtmtechltd/terraform-provider-azuread/tree/gm-add-authoritative-approles-and-assignments )

I am conducting some tests now and going to have to work out how to write tests for it.

But I thought you would be interested in the update.

I will let you know outcomes soon.

gtmtech commented 2 years ago

Hi @manicminer

I've done some further updates and I now have an AWS Gallery app starting up complete with approle assignments.

I want to write some tests, but I wanted some information from you - do the tests run against an actual AzureAD installation or a mock AzureAD framework? It looked a little like it was a real one, but I wanted to check- thanks