[Design] Remove default security groups

[gautam-nutalapati]

Proposal for #3387

Use deny_default_security_group key to indicate service should not use default security group.

Example:

    network:
      vpc:
        placement: 'private'
        security_groups: ['sg-xxxxxxxxxxxxxxxxx']
        deny_default_security_group: true

    network:
      vpc:
        deny_default_security_group: true

• When deny_default_security_group is false, Current behavior is not modified and Default EnvironmentSecurityGroup is applied.
• When deny_default_security_group or vpc OR network keys are not present in yaml, deny_default_security_group is false by default. Current behavior is not modified, and Default EnvironmentSecurityGroup is applied.
• When deny_default_security_group is true, Default EnvironmentSecurityGroup is NOT applied.

[Lou1415926]

Hello @gautam-nutalapati ! Thank you very much for the proposal!

I see that you are looking to remove the EnvironmentSecurityGroup from a service. If I'm not mistaken, you'd like to scope down the service's permissions that's set up by security group.

With this sg removed, the service would not be able to access a majority of the environment resources, neither would it be accessible from most of these environment resources. Does this align with your use case?

An alternative I could think of is modifying the ingress of the EnvironmentSecurityGroup 🤔 . This would affect all services deployed in the environment, though.

[gautam-nutalapati]

Hi @Lou1415926!

Your understanding is correct. We would like to scope down service permissions. We want to prevent egress, and only for a few services. For rest of the services, svc-to-svc and egress communications are allowed (current approach of Copilot).

As this is for subset of services, a service specific security group that allows ingress from EnvironmentSecurityGroup, and egress to VPC endpoints is created in add-ons. We can also allow egress to EnvironmentSecurityGroup if needed (just not our use case).

This can be also useful to control which services can talk with each other. (avoiding EnvironmenSecurityGroup for services that just need to talk to each other) etc.

We don't want to prevent egress for all services. But if users want to prevent egress completely, like you said, we can modify EnvironmenSecurityGroup security group. Or use this flag along with a custom EnvironmenSecurityGroup specified in network/vpc/security_groups.

I think service specific flag approach is more flexible. Update: Modifying EnvironmenSecurityGroup security group would be cleaner to control egress globally, may be it can be part of EnvironmentManifest.

[Lou1415926]

@gautam-nutalapati Thanks a lot for the detailed explanation! What you described totally makes sense to me. For users that want to scope down their service security group, they should be able to remove the default EnvironmenSecurityGroup.

I have an alternative manifest design:

// Example 1
network:
   vpc:
      security_groups: 
          deny_default: true
          groups: ['my-sg-1', 'my-sg-2']

This design groups deny_default under security_groups to reduce repetition.

With this design, security_groups will be either a map (like the example), or an array (like what it originally is). For example all of "Example 2", "Example 3" and "Example 4" are considered valid manifests and mean the same thing:

// Example 2
network:
   vpc:
      security_groups:  ['my-sg-1', 'my-sg-2']

// Example 3
network:
   vpc:
      security_groups: 
          groups: ['my-sg-1', 'my-sg-2']

// Example 4
network:
   vpc:
      security_groups: 
          deny_default: false
          groups: ['my-sg-1', 'my-sg-2']

If you like this alternative, I'm happy to give you pointers on how Copilot handles YAML fields like this - that accept either type X or Y!

[gautam-nutalapati]

Thank you for the proposal! I like this approach. Please point me to how copilot handles such fields.

[Lou1415926]

Yayy thank you! Apologize in advance for the length 🙇🏼‍♀️ - I want to be as detailed and clear as I can!

We have a bunch of manifest fields that can be either type X or Y - for example, image.build can take either a string or a map of arguments:

// Taking a string.
image:
  build: Dockerfile

// Taking a map of args.
image:
  build:
    dockerfile: Dockerfile
    target: build-stage

The implementation can be broken down into 5 major parts:

1. Define the type AorB that can take either type X or Y. In our case, it's something like SecurityGroupsIDsOrConfig.
2. Implement special UnmarshalYAML logic to unmarshal either type X or Y into the AorB.
3. Implement special environment override logic for AorB.
4. Implement Validate for AorB (no-op for in this case).
5. Translate the manifest configuration to template.

1-3 are needed because the field is of type AorB; 4 and 5 apply to generally all manifest fields, doesn't matter if it's AorB or not.

I will go through each of these parts individually, taking image.build as the example!

[Lou1415926]

1. Define the type AorB that can take either type X or Y.

Underneath, image.build is really a struct that has two fields: https://github.com/aws/copilot-cli/blob/0bea71053c1f260ad4f832d7cb550353941f5d51/internal/pkg/manifest/workload.go#L316

This struct BuildArgsOrString is referenced here: https://github.com/aws/copilot-cli/blob/0bea71053c1f260ad4f832d7cb550353941f5d51/internal/pkg/manifest/workload.go#L112

2. Special UnmarshalYAML logic

BuildArgsOrString has this special unmarshal logic so that either BuildString or BuildArgs is marshaled : https://github.com/aws/copilot-cli/blob/0bea71053c1f260ad4f832d7cb550353941f5d51/internal/pkg/manifest/workload.go#L331

so that when the yaml package reads "builds" from the manifest, it'll call this customized UnmarshalYAML method.

3. Special environment override logic

Copilot has this environments field for users to override the top-level manifest given an environment - you're probably familiar with this already! Fields of type AorB introduces complexity to this overriding procedure. For example:

image:
  build: Dockerfile

environments:
  test:
    image:
      build:
        dockerfile: AnotherDockerfile
        target: build-stage

Without special logic to handle environment override (we use mergo to do the override), the manifest would result in

Image.Build.BuildString = Dockerfile
Image.Build.BuildArgs.Dockerfile = AnotherDockerfile
Image.Build.BuildArgs.Target = BuildStage

which is ambiguous - we don't know whether Image.Build.BuildString is the override, or Image.Build.BuildArgs is the override.

To do this, we define buildArgsOrStringTransformer: https://github.com/aws/copilot-cli/blob/0bea71053c1f260ad4f832d7cb550353941f5d51/internal/pkg/manifest/transform.go#L80

The logic is very similar across the board for all AorB types.

And then we add it to the list of transformers we want to apply: https://github.com/aws/copilot-cli/blob/0bea71053c1f260ad4f832d7cb550353941f5d51/internal/pkg/manifest/transform.go#L15

This way, mergo will pick it up when it does the overriding: https://github.com/aws/copilot-cli/blob/0bea71053c1f260ad4f832d7cb550353941f5d51/internal/pkg/manifest/lb_web_svc.go#L205

4. Implement Validate.

I think this can be a no-op for our SecurityGroupsIDsOrConfig, similar to https://github.com/aws/copilot-cli/blob/9383fb10a96695186260f6045ade6fc4d50017b2/internal/pkg/manifest/validate.go#L729

5. Translate from manifest to CFN template.

This is mostly done in https://github.com/aws/copilot-cli/blob/mainline/internal/pkg/deploy/cloudformation/stack/transformers.go. Unlike Validate and environment overrides, there isn't a standardized way to do this for all AorB types. Feel free to go with whatever makes sense to you! But just for your reference, here are two related examples:

1. https://github.com/aws/copilot-cli/blob/9383fb10a96695186260f6045ade6fc4d50017b2/internal/pkg/deploy/cloudformation/stack/transformers.go#L704
2. https://github.com/aws/copilot-cli/blob/9383fb10a96695186260f6045ade6fc4d50017b2/internal/pkg/deploy/cloudformation/stack/transformers.go#L881