fix: security(charts): add IMDS egress NetworkPolicy, remove IMDSv1 g…#192
fix: security(charts): add IMDS egress NetworkPolicy, remove IMDSv1 g…#192sk-portkey wants to merge 1 commit into
Conversation
…uidance, tighten Bedrock IAM
There was a problem hiding this comment.
Pull request overview
This PR hardens the Helm charts and documentation against SSRF-to-IMDS credential theft and improves AWS Bedrock IAM guidance by promoting least-privilege and safer auth patterns (IRSA/IMDSv2).
Changes:
- Add optional egress
NetworkPolicymanifests (disabled by default) to block AWS IMDS access from pods. - Add commented “hardened defaults” guidance for pod/container security contexts in chart values.
- Update Bedrock and chart docs to recommend IRSA, require IMDSv2-only, and scope Bedrock IAM
Resourceto specific model/profile ARNs.
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 7 comments.
Show a summary per file
| File | Description |
|---|---|
| charts/portkey-gateway/values.yaml | Adds securityContext hardening guidance and introduces networkPolicy values. |
| charts/portkey-gateway/templates/networkpolicy.yaml | New template to block egress to IMDS for gateway + dataservice when enabled. |
| charts/portkey-gateway/docs/Bedrock.md | Tightens Bedrock IAM examples and adds security guidance around IRSA/IMDSv2. |
| charts/portkey-gateway/README.md | Updates IAM/Bedrock setup docs (IRSA recommended, IMDSv2-only guidance). |
| charts/portkey-app/values.yaml | Introduces networkPolicy values and securityContext hardening guidance. |
| charts/portkey-app/templates/networkpolicy.yaml | New template to block egress to IMDS for all chart pods when enabled. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| - 169.254.169.254/32 | ||
| {{- if .Values.networkPolicy.blockLinkLocal }} | ||
| - 169.254.0.0/16 | ||
| {{- end }} |
There was a problem hiding this comment.
When blockLinkLocal is enabled, this policy denies all 169.254.0.0/16 egress, which can break NodeLocal DNSCache (169.254.20.10) and other legitimate link-local endpoints. If you want to keep this option, consider adding an explicit allow rule for NodeLocal DNSCache (UDP/TCP 53) or documenting that users must disable NodeLocal DNSCache / provide their own egress exceptions.
| # Requires a NetworkPolicy-enforcing CNI (Calico, Cilium, etc.). | ||
| networkPolicy: | ||
| enabled: false | ||
| # Also block the full link-local range 169.254.0.0/16. |
There was a problem hiding this comment.
blockLinkLocal: true blocks all 169.254.0.0/16, which can break NodeLocal DNSCache (commonly 169.254.20.10) and other required link-local endpoints. Consider adding a warning here (and/or a way to allow specific link-local IPs) so enabling this doesn’t unexpectedly break DNS.
| # Also block the full link-local range 169.254.0.0/16. | |
| # Also block the full link-local range 169.254.0.0/16. | |
| # Warning: enabling this can break NodeLocal DNSCache (commonly 169.254.20.10) | |
| # and other required link-local endpoints. Only enable after verifying your | |
| # cluster does not depend on link-local services for DNS or other networking. |
| {{- toYaml . | nindent 14 }} | ||
| {{- end }} |
There was a problem hiding this comment.
Same as the gateway chart: ipBlock.cidr: 0.0.0.0/0 is IPv4-only, so any IPv6 CIDRs placed into additionalBlockedCidrs would render an invalid except list and the NetworkPolicy will be rejected by the API server. Consider documenting IPv4-only, validating input, or adding a separate IPv6 egress rule (::/0) for dual-stack support.
| {{- toYaml . | nindent 14 }} | |
| {{- end }} | |
| {{- range . }} | |
| {{- if contains ":" . }} | |
| {{- fail (printf "networkPolicy.additionalBlockedCidrs entry %q is IPv6, but charts/portkey-app/templates/networkpolicy.yaml currently renders an IPv4-only ipBlock.cidr (0.0.0.0/0). Use only IPv4 CIDRs here." .) }} | |
| {{- end }} | |
| - {{ . }} | |
| {{- end }} | |
| {{- end }} |
| - 169.254.169.254/32 | ||
| {{- if .Values.networkPolicy.blockLinkLocal }} | ||
| - 169.254.0.0/16 | ||
| {{- end }} |
There was a problem hiding this comment.
With blockLinkLocal enabled, this denies all 169.254.0.0/16 egress. That can break NodeLocal DNSCache (169.254.20.10) and other legitimate link-local traffic. Consider adding an explicit allow rule for NodeLocal DNSCache (ports 53) or documenting the limitation clearly.
| # Additional CIDRs to deny from pod egress (e.g., internal metadata services). | ||
| additionalBlockedCidrs: [] | ||
| # - 169.254.170.2/32 # ECS task metadata endpoint | ||
| # - fd00:ec2::254/128 # IMDS IPv6 |
There was a problem hiding this comment.
The values comment suggests adding an IPv6 CIDR (e.g., fd00:ec2::254/128) to networkPolicy.additionalBlockedCidrs, but the rendered policy uses an IPv4 ipBlock (cidr: 0.0.0.0/0). Kubernetes requires ipBlock.except entries to be the same IP family as cidr, so any IPv6 entry here will make the NetworkPolicy invalid and fail to apply. Consider either removing the IPv6 example / explicitly documenting IPv4-only, or adding dual-stack support (separate IPv6 egress rule and separate v6 blocked CIDRs).
| # Additional CIDRs to deny from pod egress (e.g., internal metadata services). | |
| additionalBlockedCidrs: [] | |
| # - 169.254.170.2/32 # ECS task metadata endpoint | |
| # - fd00:ec2::254/128 # IMDS IPv6 | |
| # Additional IPv4 CIDRs to deny from pod egress (e.g., internal metadata services). | |
| # Do not include IPv6 CIDRs here unless the templates are updated with separate IPv6 rules. | |
| additionalBlockedCidrs: [] | |
| # - 169.254.170.2/32 # ECS task metadata endpoint |
| networkPolicy: | ||
| # Set to true to install egress NetworkPolicies for gateway and dataservice. | ||
| enabled: false | ||
| # Also block the full link-local range 169.254.0.0/16. |
There was a problem hiding this comment.
blockLinkLocal: true blocks all 169.254.0.0/16, which can break clusters using NodeLocal DNSCache (commonly 169.254.20.10) and any other required link-local endpoints. Either add a prominent warning here (including the NodeLocal DNSCache caveat) or provide an explicit allow exception for required link-local IPs when blockLinkLocal is enabled.
| # Also block the full link-local range 169.254.0.0/16. | |
| # WARNING: When enabled, this blocks the full link-local range 169.254.0.0/16. | |
| # This can break clusters that rely on link-local endpoints such as NodeLocal | |
| # DNSCache (commonly 169.254.20.10) and other required local services. | |
| # Only enable this if your cluster does not depend on such endpoints, or if | |
| # required exceptions are handled elsewhere in your NetworkPolicy configuration. |
| - to: | ||
| - ipBlock: | ||
| cidr: 0.0.0.0/0 | ||
| except: | ||
| - 169.254.169.254/32 | ||
| {{- if .Values.networkPolicy.blockLinkLocal }} | ||
| - 169.254.0.0/16 | ||
| {{- end }} | ||
| {{- with .Values.networkPolicy.additionalBlockedCidrs }} | ||
| {{- toYaml . | nindent 14 }} | ||
| {{- end }} |
There was a problem hiding this comment.
ipBlock.cidr is IPv4-only (0.0.0.0/0), but additionalBlockedCidrs can contain IPv6 CIDRs; Kubernetes requires all except entries to be in the same IP family as the parent cidr, otherwise the NetworkPolicy is rejected. Consider validating additionalBlockedCidrs to IPv4-only (and fail the render on IPv6), or add a second IPv6 ipBlock rule (::/0) with its own except list for dual-stack clusters.
…uidance, tighten Bedrock IAM