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

Added support for SSM connections #10

Merged
merged 3 commits into from
Mar 24, 2024
Merged

Added support for SSM connections #10

merged 3 commits into from
Mar 24, 2024

Conversation

kieranbrown
Copy link
Contributor

@kieranbrown kieranbrown commented Jan 23, 2024

Unsure if you want this configurable by a variable? Considering SSM may become the only method to connect by default (AndrewGuenther/fck-nat#48) I figured there was no harm in attaching the SSM policy by default.

For now, this will only be relevant for users building their own AMI based on your PR to add SSM support.

@RaJiska
Copy link
Owner

RaJiska commented Feb 25, 2024

Thanks for the PR @kieranbrown . Indeed, adding SSM permissions would be necessary once this becomes the default in the upstream fck-nat project. However I am a bit uncomfortable with using the AWS' managed policy for this project as the policy has too many permissions.

When SSM becomes the default, I think it would be better either to make a trimmed policy containing only the IAM actions for SSM-messages required for remote shell, or alternatively at the very least exposing a variable controlling the policy ARN to let the user chose a policy that suit their needs.

@kieranbrown kieranbrown force-pushed the ssm branch 4 times, most recently from 34a72d3 to f488067 Compare February 27, 2024 23:42
@kieranbrown
Copy link
Contributor Author

kieranbrown commented Feb 27, 2024

@RaJiska I've updated the PR to use least privilege and added a variable to disable for those who don't want it.

There are a few approaches we can take for those who have more complex requirements, personally I'm leaning towards outputting a role_id or role_name and allowing users to attach managed policies or inline policies themselves outside of the module - I know the role name is just var.name but it saves people needing to use depends_on to wait the for role creation if they can reference it directly.

Curious to hear your thoughts on this, I'm happy to work on it in a separate PR as I don't think it's absolutely necessary for this to be merged.

@RaJiska RaJiska merged commit 2b147f0 into RaJiska:main Mar 24, 2024
1 check passed
@RaJiska
Copy link
Owner

RaJiska commented Mar 24, 2024

LGTM, thanks for the contribution @kieranbrown .

I think outputting the role name and ARN is definitely a good idea regardless of whether the user wishes to add policies or not as the end user may make alternative use of it (e.g: storing in parameter store).

Though I'm more for adopting a minimalist approach in regards of the number of variables exposed to the user as in my opinion, the goal of this project is to mimic NAT Gateway, and make this as dumb as possible, therefore removing as much complexity as possible. In the future I may wish to remove the attach_ssm_policy variable and just have it enabled by default and make SSH off by default.

@kieranbrown kieranbrown deleted the ssm branch August 6, 2024 14:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants