AWS Enclave Deployment Test #8
Conversation
WalkthroughReorders an enum variant in the enclave vsock server, removes the ingress listen_port from the enclave manifest and adds allowed egress endpoints, updates the CDK stack to reference an existing ECR repository and image tag, swaps the enclave Docker runtime base image, and expands EC2/CDK deployment docs. (37 words) Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes
Possibly related PRs
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✨ Finishing touches🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: Organization UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
🔇 Additional comments (3)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
wksantiago
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
keep-enclave/enclave/src/vsock_server.rs (1)
15-29: Verify deployment scope before shipping enum reordering.Postcard encodes enum discriminants based on variant position, so the variant reordering changes discriminant values:
ImportFrostKeymoves from position 4 to position 8,Signfrom 5 to 4, etc. This breaks deserialization of messages serialized with the previous enum order.The commit message "Fix enclave protocol enum order" indicates this reordering was intentional. However, confirm:
- Was the previous enum order (from fef7ef6) ever deployed to clients or production enclaves?
- If yes, are all clients and enclaves upgraded atomically to ensure version alignment?
- If this is a pre-release correction, document that explicitly to prevent future confusion.
Without this context, the change risks silent deserialization failures or version mismatch errors in cross-component communication.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
keep-enclave/enclave/src/vsock_server.rskeep-enclave/enclaver.yaml
💤 Files with no reviewable changes (1)
- keep-enclave/enclaver.yaml
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: build
🔇 Additional comments (1)
keep-enclave/enclave/src/vsock_server.rs (1)
218-243: Match statement correctly handles all enum variants.The exhaustive match properly routes each
EnclaveRequestvariant to its corresponding handler. Rust's pattern matching is unaffected by enum variant reordering, so the logic remains correct.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
deploy/cdk/keep_cdk/keep_stack.py (2)
24-29: Avoid using:latesttag for production deployments.Using the
latesttag makes deployments non-deterministic since the underlying image can change without updating the stack. This can lead to unexpected behavior during scaling events or instance replacements.🔎 Suggested approach
Consider using a specific image tag (e.g., Git SHA, semantic version) or pass the tag as a stack parameter:
+from aws_cdk import CfnParameter + +# In __init__: + image_tag = CfnParameter( + self, "ImageTag", + type="String", + default="latest", + description="Docker image tag for the enclave image" + ) + enclave_repo = aws_ecr.Repository.from_repository_name( self, "KeepEnclaveRepo", repository_name="keep-enclave", ) - enclave_image_uri = f"{enclave_repo.repository_uri}:latest" + enclave_image_uri = f"{enclave_repo.repository_uri}:{image_tag.value_as_string}"
88-93: Redundant ingress rule.The VPC CIDR rule on line 88-90 is redundant since the
any_ipv4()rule on lines 91-93 already permits all IPv4 traffic on port 443. Consider removing the more specific rule unless kept intentionally for documentation purposes.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
deploy/cdk/keep_cdk/keep_stack.pykeep-enclave/build/Dockerfile.enclave
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: build
🔇 Additional comments (5)
keep-enclave/build/Dockerfile.enclave (1)
18-18: LGTM! Using AWS ECR Public Gallery is preferred for AWS deployments.This avoids Docker Hub rate limits and ensures consistent image availability when deploying to AWS infrastructure.
deploy/cdk/keep_cdk/keep_stack.py (4)
145-146: Hardcoded key pair name requires pre-existing key.The key pair
keep-enclavemust exist in the target AWS account before deployment, otherwise CloudFormation will fail. Consider parameterizing this or documenting the prerequisite.
148-159: LGTM on the CFN override approach.Using
add_property_overridefor NetworkInterfaces is the correct pattern when you need to associate both a public IP and security groups, since the L2 construct doesn't support this combination directly.
178-180: Verify architectural intent: public instances with internal NLB.The ASG is now in public subnets with public IPs, but the NLB remains internal (
internet_facing=False) in private subnets. This is an unusual pattern—typically, public-facing instances would use a public NLB, or internal services would keep instances in private subnets.Is this configuration intentional for testing purposes? For production, consider whether the instances truly need public IPs or if the NLB should be internet-facing.
112-112: LGTM on grant_pull usage.Correctly grants ECR pull permissions to the instance role for the referenced repository.
There was a problem hiding this comment.
Actionable comments posted: 3
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
deploy/cdk/keep_cdk/keep_stack.py
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: build
🔇 Additional comments (4)
deploy/cdk/keep_cdk/keep_stack.py (4)
9-9: LGTM!The addition of
aws_ecrimport is necessary for referencing the existing ECR repository.
109-109: LGTM!The grant correctly uses the new
enclave_reporeference, consistent with the architectural change to use an existing ECR repository.
124-124: LGTM!The mapping correctly references the
enclave_image_urivariable, consistent with the ECR repository changes.
176-176: Reconsider the architectural implications of placing instances in PUBLIC subnets with configured VPC endpoints.The change to
SubnetType.PUBLICcreates several architectural concerns, though some of the original routing concerns require clarification:
VPC Endpoint accessibility - Interface endpoints (KMS, ECR) use DNS resolution: VPC endpoints for KMS and ECR are powered by AWS PrivateLink, which enables private access through private IP addresses via interface VPC endpoints. When private DNS is enabled on these endpoints, applications use them by default. This means instances in PUBLIC subnets can access these endpoints via DNS resolution regardless of subnet placement. The original concern about routing is only applicable to gateway endpoints (S3), not these interface endpoints.
Cost implications remain: Even if endpoints are accessible, interface VPC endpoints incur a fixed cost of $9.36/month before data processing fees. If instances in public subnets route to AWS services via the internet gateway instead of using these endpoints, the endpoints become a sunk cost while adding internet egress charges.
Architectural inconsistency: The Network Load Balancer is internal (
internet_facing=False) in private subnets, but ASG instances are now publicly accessible. This creates asymmetry where instances are directly reachable from the internet but the load balancer is not.Security consideration: Public subnets with direct internet access increase the attack surface compared to private instances accessed through controlled entry points.
For production deployments, consider reverting to
PRIVATE_WITH_EGRESSand using the NLB as the sole internet-facing component, or making the NLB internet-facing if direct instance access is required.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
docs/ENCLAVE.md (1)
99-99: Consider adding a note about AWS account and region substitution.The command references
<account>as a placeholder, but users might benefit from explicit guidance on how to retrieve their AWS account ID.🔎 Suggested documentation enhancement
Consider adding a note before line 99:
Note: Replace `<account>` with your AWS account ID. You can retrieve it with: ```bash aws sts get-caller-identity --query Account --output text</details> </blockquote></details> </blockquote></details> <details> <summary>📜 Review details</summary> **Configuration used**: Organization UI **Review profile**: CHILL **Plan**: Pro <details> <summary>📥 Commits</summary> Reviewing files that changed from the base of the PR and between 7a6e6a8497e59d3dd77c5b9f0c3b90e673835b68 and a46e4ec4e6fa1f79f47fcb08b3d58f06c182c329. </details> <details> <summary>📒 Files selected for processing (2)</summary> * `deploy/cdk/keep_cdk/keep_stack.py` * `docs/ENCLAVE.md` </details> <details> <summary>🔇 Additional comments (15)</summary><blockquote> <details> <summary>docs/ENCLAVE.md (7)</summary><blockquote> `64-75`: **LGTM!** The deployment flow correctly demonstrates docker save/load workflow for transferring the enclave image to EC2. The instructions appropriately reference the CDK and manual deployment sections for instance provisioning. --- `102-102`: **Documentation correctly reflects that ECR is no longer provisioned by CDK.** This change aligns with the code updates where the ECR repository is now referenced from an existing repository rather than being created by the stack. --- `106-114`: **LGTM!** The IAM role creation steps are correct and follow AWS best practices. The instance profile setup is necessary for EC2 instances to assume the role, and exporting `EC2_ROLE_ARN` facilitates its use in subsequent commands. --- `132-132`: **LGTM!** Adding the IAM instance profile to the EC2 launch command is essential for the instance to access KMS and ECR. This aligns with the CDK stack changes and manual deployment guidance. --- `137-137`: **LGTM!** The region-specific AMI note is helpful and provides users with a command to find the appropriate AMI for their region. This prevents hardcoding region-specific values and improves the documentation's portability. --- `168-183`: **LGTM!** The updated KMS policy guidance with explicit placeholder replacement instructions is clear and comprehensive. The addition of `echo $EC2_ROLE_ARN` reference helps users retrieve the correct value. The file path update to `keep-enclave/build/kms-policy.json` is also correct. --- `83-91`: **ECR repository name matches CDK stack configuration.** The repository name `keep-enclave` in the documentation (line 86) is consistent with the CDK stack reference in keep_stack.py (line 31), and the image push workflow is correct. Ensure users have the necessary IAM permissions to create repositories and push images. </blockquote></details> <details> <summary>deploy/cdk/keep_cdk/keep_stack.py (8)</summary><blockquote> `9-9`: **LGTM!** The import change from `aws_ecr_assets` to `aws_ecr` is correct and necessary for the switch from building images in CDK to referencing an existing ECR repository. --- `21-23`: **LGTM!** The image tag context retrieval with a sensible default (`v1.0.0`) provides flexibility for deployments while avoiding the `:latest` anti-pattern flagged in previous reviews. The comment clearly documents how to override the default. --- `92-97`: **LGTM!** The security group configuration correctly restricts HTTPS traffic to within the VPC CIDR block, addressing the previous security concern about internet exposure. The descriptive comment clarifies the intent. --- `113-113`: **LGTM!** The grant change from `enclave_image.repository.grant_pull(role)` to `enclave_repo.grant_pull(role)` is correct and necessary for the new repository reference pattern. --- `128-128`: **LGTM!** The mapping update correctly uses the new `enclave_image_uri` variable, which includes the configurable image tag. This ensures the user data script receives the correct image URI. --- `146-148`: **LGTM!** Adding the `security_group` parameter to the LaunchTemplate and documenting the SSM-based access approach addresses the previous concern about the hardcoded key pair. This eliminates the deployment dependency on a pre-existing SSH key. --- `194-194`: **LGTM!** Adding the ImageTag as a CloudFormation output provides visibility into which image version is deployed, supporting traceability and troubleshooting. --- `28-33`: **ECR repository setup is already documented.** The code correctly uses `from_repository_name` to reference the "keep-enclave" repository, which requires it to exist before deployment. The documentation at docs/ENCLAVE.md (lines 83-92) already provides clear setup instructions, including the `aws ecr create-repository --repository-name keep-enclave` command as a prerequisite step before running `cdk deploy`. No changes needed. </blockquote></details> </blockquote></details> </details> <!-- This is an auto-generated comment by CodeRabbit for review status -->
… open ingress and key_name
wksantiago
force-pushed
the
AWS-deploy
branch
from
a46e4ec to
e3e7379
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
docs/ENCLAVE.md (1)
83-91: Consider adding a note about replacing placeholders.The commands use
<account>as a placeholder for the AWS account ID. While this is a common convention, consider adding a brief note (e.g., "Replace<account>with your AWS account ID") to make it explicit for users who may be new to AWS.🔎 Suggested addition
Create ECR repository and push enclave image first: + +> **Note:** Replace `<account>` with your AWS account ID in the commands below. ```bash
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
.gitignoredeploy/cdk/keep_cdk/keep_stack.pydocs/ENCLAVE.mdkeep-enclave/build/Dockerfile.enclavekeep-enclave/enclave/src/vsock_server.rskeep-enclave/enclaver.yaml
💤 Files with no reviewable changes (1)
- keep-enclave/enclaver.yaml
🚧 Files skipped from review as they are similar to previous changes (3)
- keep-enclave/build/Dockerfile.enclave
- keep-enclave/enclave/src/vsock_server.rs
- .gitignore
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: build
🔇 Additional comments (8)
docs/ENCLAVE.md (3)
64-77: LGTM! Clear deployment flow guidance.The addition of a note directing users to provision an instance first (via CDK or Manual) before using Enclaver is a helpful clarification that aligns with the updated deployment workflow.
106-138: LGTM! Comprehensive manual deployment guidance.The manual deployment section provides clear, step-by-step instructions for IAM setup, SSH key creation, and EC2 launch. The inclusion of the IAM instance profile in the launch command and the region-specific AMI guidance are particularly helpful additions.
168-182: LGTM! Improved KMS setup documentation.The expanded placeholder replacement instructions are thorough and user-friendly. Setting
KMS_KEY_IDas an environment variable and providing clear guidance for each placeholder (including how to retrieve values) will help users avoid configuration errors.deploy/cdk/keep_cdk/keep_stack.py (5)
21-33: LGTM! Repository-based image reference with configurable tag.The shift from
DockerImageAssettoRepository.from_repository_namecorrectly implements the repository-based workflow. Theimage_tagcontext parameter with a sensible default ("v1.0.0") enables version-controlled deployments, and the documentation properly reflects the prerequisite of creating the ECR repository and pushing the image before runningcdk deploy.
92-97: LGTM! Security restriction to VPC traffic.The security group rule correctly restricts HTTPS ingress to the VPC CIDR block, allowing only internal NLB traffic to reach the enclave instances. This is a significant security improvement over exposing instances directly to the internet.
113-113: LGTM! Correct permission grant for repository access.The update to use
enclave_repo.grant_pull(role)correctly grants ECR pull permissions to the instance role, aligning with the repository-based approach.
146-148: LGTM! Improved access control via SSM Session Manager.Removing the hardcoded SSH key and explicitly attaching the security group to the LaunchTemplate are good improvements. Relying on SSM Session Manager (enabled via the
AmazonSSMManagedInstanceCorerole) eliminates the dependency on pre-existing key pairs and provides secure, auditable access without exposing SSH ports.
194-194: LGTM! Useful CloudFormation output for tracking deployed version.Adding the
ImageTagoutput enables operators to verify which container image version was deployed, improving operational visibility and version tracking.
Summary by CodeRabbit
Chores
Refactor
Documentation
✏️ Tip: You can customize this high-level summary in your review settings.