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

Remove ssh-egress security group rule #179

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

m-wynn
Copy link

@m-wynn m-wynn commented May 10, 2024

what

  • Delete the unnecessary "ssh-egress" security-group rule. It does not provide anything related to SSH.

⚠️ This might break environments for people who are accidentally relying on the wide security group rules for non-ssh outbound access. However it will allow others to be more secure and tighten down their security posture.

why

AWS Security Groups are stateful, meaning that any inbound traffic that is allowed will automatically permit the return outbound traffic, regardless of outbound rules. Security Groups track TCP connections in order to determine which packets are responses to allowed inbound traffic.

When you establish an SSH connection, you're initiating an inbound connection to the server. The server's response to your connection, as well as any subsequent communication, is considered outbound traffic from the perspective of the security group.

Because of the stateful nature of AWS Security Groups, once you've allowed inbound SSH traffic, the return traffic (outbound from the server) is automatically allowed, even if your outbound rules don't explicitly permit it.

Therefore, an outbound rule that allows traffic on all ports is not necessary for the operation of inbound SSH.

references

AWS Security Groups are stateful, meaning that any inbound traffic that
is allowed will automatically permit the return outbound traffic,
regardless of outbound rules.

When you establish an SSH connection, you're initiating an inbound
connection to the server. The server's response to your connection, as
well as any subsequent communication, is considered outbound traffic
from the perspective of the security group.

Because of the stateful nature of AWS Security Groups, once you've
allowed inbound SSH traffic, the return traffic (outbound from the
server) is automatically allowed, even if your outbound rules don't
explicitly permit it.

Therefore, an outbound rule that allows traffic on all ports is not
necessary for the operation of inbound SSH.

This might break environments for people who are accidentally relying on
the wide security group rules for non-ssh outbound access.  However it
will allow others to be more secure and tighten down their security
posture.
@m-wynn m-wynn requested review from a team as code owners May 10, 2024 19:06
@m-wynn m-wynn requested review from kevcube and jamengual May 10, 2024 19:06
@mergify mergify bot added the triage Needs triage label May 10, 2024
Copy link
Contributor

@Nuru Nuru left a comment

Choose a reason for hiding this comment

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

@m-wynn Thank you for this PR.

When you say:

An outbound rule that allows traffic on all ports is not necessary for the operation of inbound SSH.

I believe you are correct. However, I'm not sure that users are using it that way. When the source is a security group (e.g. a bastion host), they may be intending to allow a user from that SG to run other commands on the node group that access that SG, such as curl or ftp. So, although this is indeed tightening a loophole, it nevertheless is a breaking change. Would you mind explaining this in your PR comments, and explaining how to restore unlimited egress to the source SG or CIDR if that is desired?

(Reverting will be a lot easier if you make the egress rule optional with a new variable ssh_allow_all_egress. I propose you make that a boolean variable that defaults to null. When null, all egress is allowed when ssh_access_security_group_ids is supplied but not allowed if it is not, and the source CIDR is wide open. How does that sound to you?)

Also, we don't have a test for SSH access. Would you be willing to add one? (It requires you know go and Terratest, and is a non-trivial amount of work, so I understand if you feel you can't or don't want to do it.)

@Nuru Nuru added minor New features that do not break anything needs-test Needs testing and removed triage Needs triage labels May 10, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
minor New features that do not break anything needs-test Needs testing
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants