search

aws / copilot-cli

The AWS Copilot CLI is a tool for developers to build, release and operate production ready containerized applications on AWS App Runner or Amazon ECS on AWS Fargate.
https://aws.github.io/copilot-cli/
Apache License 2.0
3.48k stars 400 forks source link

Feature Request Follow session-manager-plugin usage #2711

Open somen440 opened 3 years ago

somen440 commented 3 years ago

When the AWS CLI exec session-manager-plugin, it passes 7 args. https://github.com/aws/aws-cli/blob/45b0063b2d0b245b17a57fd9eebd9fcc87c4426a/awscli/customizations/ecs/executecommand.py#L105

copilot-cli, it passes 4 args https://github.com/aws/copilot-cli/blob/mainline/internal/pkg/exec/ssm_plugin.go#L55

In session-manager-plugin, it is in the state of IsAwsCliUpgradeNeeded with 4 arguments as LegacyArgumentLength. https://github.com/aws/session-manager-plugin/blob/e4ad4017df969b298c3e263f0c8ce9881c0b0a79/src/sessionmanagerplugin/session/session.go#L136

At this time, ProcessKMSEncryptionHandshakeAction will display an error https://github.com/aws/session-manager-plugin/blob/65933d1adf368d1efde7380380a19a7a691340c1/src/datachannel/streaming.go#L832

Specifically, it occurs in a valid cluster with kms-encrypted logs as follows: https://docs.aws.amazon.com/cdk/api/latest/docs/aws-ecs-readme.html#enabling-logging

I'm sorry I'm not familiar with copilot, Similar issues are occurring with other tools that utilize session-plugin-manager. How does copilot address this issue?

iamhopaul123 commented 3 years ago

Hello @somen440. You are correct: for now since Copilot doesn't create a KMS key for each environment or service, we don't use log encryption for the ecs exec log. Do you want your exec log to be encrypted or did you encounter any issue when using Copilot?

somen440 commented 3 years ago

Thank you !! @iamhopaul123.

we don't use log encryption for the ecs exec log.

I understand this.

Do you want your exec log to be encrypted

For consistent compliance, I wanted log encryption.

I have an additional question. is there usually no need for encryption in the exec context? e.g. encryption doesn't make sense if the S3 bucket is private.

iamhopaul123 commented 3 years ago

Yeah but we currently don't allow users to specify the destination of their exec logs. I'll put it in our backlog for encrypting the exec logs. Thank you for reporting this!

iamhopaul123 commented 3 years ago

Also would you mind to share with us why you want the exec logs to be encrypted (we also don't do log encryption for service logs)?

somen440 commented 3 years ago

Also would you mind to share with us why you want the exec logs to be encrypted (we also don't do log encryption for service logs)?

very sorry. It's very difficult to answer this question. I'm not familiar with encryption myself, and I don't understand the question correctly.

I write the background.

I learned about bucket encryption by enabling exec log with aws-cdk. The sample below uses s3 bucket encryption with kms by default. https://github.com/aws/aws-cdk/tree/master/packages/%40aws-cdk/aws-ecs#enabling-logging

I ran into a problem at this time when using an auxiliary tool for ecs that wasn't awscli. content is "Please upgrade to latest version of AWS CLI".

I was very confused (because I was asked to upgrade awscli even though I wasn't using awscli), but I found that it was in the way session-manager-plugin handles arguments.

In this issue, copilot handled the same arguments, so I wanted to hear the policy as copilot. I think the title was inappropriate in that respect. very sorry 😢

As in the background above, I don't have a policy for encryption as it's kind of made according to the defaults. Therefore, we would like to know the use cases that require general encryption.

My mere curiosity is, "Why didn't copilot support encryption?" (Is it because it is not necessary, or is the work simply not catching up?)

iamhopaul123 commented 3 years ago

Aha I think I got you. So it is basically because Copilot is a very opinionated tool that helps you to deploy your application easily. Instead of exposing every configurable field to users like "aws cli" or "CDK", we by default set most of the fields and only expose some of them based on customers need. We don't want users to feel overwhelmed when they try to learn how to configure the Copilot manifest. Hence, the only setting in Copilot manifest for ecs exec is exec: true to turn this feature on (see our doc here), though under the hood we set the exec log destination to be the same log group as the ECS service and add required task role permissions etc. That's also the reason why I asked for your use case for using exec log encryption, so that we can prioritize this feature request accordingly.

masteinhauser commented 2 years ago

@iamhopaul123 If this is still a feature being scoped and needs a customer use case / design partner, my team would be happy to engage. We're just starting out with Copilot to replace out (fairly) customized Elastic Beanstalk environment. ECS Exec + log encryption is of high important to us, and could be a blocker to adoption outside of our initial PoC due to SOC II requirements.

iamhopaul123 commented 2 years ago

Hello @masteinhauser. Thank you for your feedback on this! This is very helpful for us. Before we start working on this feature, could you tell us more about the requirement you'd have for the KMS key for SOC II? Several options are

  1. [Dedicated] Copilot creates an env-specific KMS key for exec encryption
  2. [Multi tenant] I want Copilot to create as few KMS keys as possible: just create a KMS key for each account region that might be shared between envs
  3. [Import existing ones] I have a KMS key and I'd like to bring my own

Or let me know if you have any other ideas!

masteinhauser commented 2 years ago

@iamhopaul123 Great question! In our case, we require #3 [Import] due to our multiple-services per-client dedicated stacks.

The option #1 [Dedicated] would work well for other teams I have been on at other companies.