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

[Spike]: API for role assignments #6636

Draft
wants to merge 11 commits into
base: main
Choose a base branch
from
Draft

Conversation

davidfowl
Copy link
Member

@davidfowl davidfowl commented Nov 9, 2024

This is a spike to explore APIs for modeling a more secure default security configuration when deploying to azure container apps. It includes a few things:

  1. A new set of APIs that azure resources to expose a way to define per reference role assignments. This allows for a least privilege access to resources:
using Azure.Provisioning.Storage;

var builder = DistributedApplication.CreateBuilder(args);

var blobs = builder.AddAzureStorage("storage")
                   .RemoveDefaultRoleAssignments()
                   .RunAsEmulator(c => c.WithLifetime(ContainerLifetime.Persistent))
                   .AddBlobs("blobs");

builder.AddProject<Projects.AzureContainerApps_ApiService>("api")
       .WithExternalHttpEndpoints()
       .WithReference(blobs)
       .WithRoleAssignments(blobs, StorageBuiltInRole.StorageBlobDataContributor);

builder.Build().Run();

In the above WithRoleAssignments allows the user to specify which roles this project/container requires what roles for a specific storage account.

The other API is RemoveDefaultRoleAssignments, which is a way to remove the default role assignments for a storage account.

  1. Each compute resource that becomes a container app, gets a roles bicep resource that allocates the managed identity and sets up specific role assignments for that identity based on both the either the per reference role assignments or the resource defaults.

  2. Role assignment scopes are resource references. This means we need the ability to get a reference to the underlying azure resource by name, so we can use an existing bicep resource reference in "roles" bicep module. This led to a strange API that was added to AzureProvisioningResource to do this in a way that avoids the compute resource needing to know how to get a reference to the underlying azure resource.

Microsoft Reviewers: Open in CodeFlow


param storage_outputs_name string

resource identity 'Microsoft.ManagedIdentity/userAssignedIdentities@2023-01-31' = {
Copy link
Member Author

Choose a reason for hiding this comment

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

We generate a module per container/project and that module has both the user assigned identity and role assignments for that compute resource.

Comment on lines 25 to 27
output id string = identity.id

output clientId string = identity.properties.clientId
Copy link
Member Author

Choose a reason for hiding this comment

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

These end up as outputs for the container app to use for both runtime and to associate this user assigned identity with the container app.

Comment on lines +11 to +13
resource storage 'Microsoft.Storage/storageAccounts@2024-01-01' existing = {
name: storage_outputs_name
}
Copy link
Member Author

Choose a reason for hiding this comment

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

We need to reference the set of resources we are creating role assignments for.

/// <param name="Description"></param>

// REVIEW: This should be part of the Azure.Provisioning APIs
public record struct RoleDefinition(string Id, string Description);
Copy link
Member Author

Choose a reason for hiding this comment

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

I think we need a single type for role definitions and then per resource types that have the well known ones. That would reduce the API explosion.

cc @tg-msft

Comment on lines +97 to +98
var account = StorageAccount.FromExisting(Infrastructure.NormalizeBicepIdentifier(Name));
account.Name = name;
Copy link
Member Author

Choose a reason for hiding this comment

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

This the strangest part of this change. We need the ability to import the underlying resource use for the role assignment given a name. Look at this to understand more.

This method is called from here

@mitchdenny
Copy link
Member

Have you tried deploying this? Based on the Bicep being generated it looks like you are deploying the -roles.module.bicep file first and then taking the user assigned identity out of that. I think that is fine for the case where container apps are only accessing other resources - but we just need to double check this model work for container app to container app deployments.

I think it'll be OK because for container app to container app communication the container app identity (resource side) is not what matters, there would need to be a different Azure Entra app registration which represents the resource (which we are going to need to support defining in the app model).

You know if we get this working it's going to take a lot of the friction out of doing S2S auth.

@davidfowl
Copy link
Member Author

Have you tried deploying this? Based on the Bicep being generated it looks like you are deploying the -roles.module.bicep file first and then taking the user assigned identity out of that. I think that is fine for the case where container apps are only accessing other resources - but we just need to double check this model work for container app to container app deployments.

Not yet, there was a bug in azd, fixed in this pr. It'll work because the identity and role assignments are part of the provision phase. I'll update the description with the deployment details.

@davidfowl
Copy link
Member Author

Deployment works!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants