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

Support EC2 instance connect #39

Merged
merged 1 commit into from
Jul 13, 2021
Merged

Conversation

samjo-nyang
Copy link
Contributor

**Issue number: #38 **

Description of changes:

  • Install ec2-instance-connect package
  • Update sshd_config file
  • Make symbolic link of persistent ssh host key to /etc/ssh in the container
  • Execute eic_harvest_hostkeys for one time

Testing done:
This patch is currently used in our internal Kubernetes cluster.

Terms of contribution:
By submitting this pull request, I agree that this contribution is dual-licensed under the terms of both the Apache License, version 2.0, and the MIT license.

@jahkeup jahkeup added the enhancement New feature or request label Jul 1, 2021
Copy link
Contributor

@zmrow zmrow left a comment

Choose a reason for hiding this comment

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

Thanks for the PR! 🎉

Since additional setup is required to use EC2 Instance Connect, we'll also want to update the documentation to point at the AWS docs re: security group guidance and IAM permissions. (https://docs.aws.amazon.com/AWSEC2/latest/UserGuide/ec2-instance-connect-set-up.html)

@tjkirch tjkirch requested a review from bcressey July 6, 2021 17:17
Copy link
Member

@jpculp jpculp left a comment

Choose a reason for hiding this comment

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

Sorry for the delayed response. After thinking about it a bit more, it might make more sense to have the userdata structure look more like:

{
   "ssh":{
      "authorized-keys":[
         "ssh-rsa EXAMPLEAUTHORIZEDPUBLICKEYHERE my-key-pair"
      ],
      "trusted-user-ca-keys":[
         "ssh-rsa EXAMPLETRUSTEDCAPUBLICKEYHERE [email protected]"
      ],
      "authorized-keys-command": "...",
      "authorized-keys-command-user": "..."
}

You could parse the authorized keys command and user and output them to their own config under /etc/ssh/sshd_config.d/. If those values are for EC2 Instance Connect you could conditionally trigger the symlink and key harvest logic you have in the current PR. We are going to double-check on our end to see if the key harvest logic is required or whether it is safe to omit. Is this something you would be open to implementing? Or would you be more comfortable with us working on it?

@jpculp
Copy link
Member

jpculp commented Jul 8, 2021

I've verified that eic_harvest_hostkeys is used for establishing trust, so we should not omit it.

@samjo-nyang
Copy link
Contributor Author

@jpculp Thanks for your review!
I re-implemented the logic using "authorized-keys-command" and "authorized-keys-command-user" userdata. Is this fine?
Also, I force-pushed this branch to squash commits and fix author info.

Copy link
Member

@jpculp jpculp left a comment

Choose a reason for hiding this comment

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

Thank you for the quick update @samjo-nyang! I think this is very close.

I'm afraid I led you astray by suggesting the switch to a separate config file. After some testing on my end, it seems that the OpenSSH in the admin container does not yet support the Include directive in the sshd_config file itself. It might be better to just append to sshd_config directly. You could have a variable like SSHD_CONFIG_FILE variable pointing to "${SSHD_CONFIG_DIR}/sshd_config".

@samjo-nyang
Copy link
Contributor Author

@jpculp Updated! (and also force-pushed)

@willthames
Copy link
Contributor

I currently get

[  321.634455] host-ctr[7917]: Failed to configure ssh authentication with user-data: {"ssh":{"authorized-keys-command":"/opt/aws/bin/eic_run_authorized_keys %u %f","authorized-keys-command-user":"ec2-instance-connect"}}
[  321.648738] host-ctr[7917]: rsa key already exists, will use existing key.
[  321.651485] host-ctr[7917]: ecdsa key already exists, will use existing key.
[  321.654770] host-ctr[7917]: ed25519 key already exists, will use existing key.
[  321.665726] host-ctr[7917]: /etc/ssh/sshd_config: line 22: Bad configuration option: Include
[  321.668964] host-ctr[7917]: /etc/ssh/sshd_config: terminating, 1 bad configuration options
[  321.708568] host-ctr[7917]: time="2021-07-09T02:56:48Z" level=fatal msg="Container admin exited with non-zero status"

I understand the Include problem is mentioned and fixed above, but the userdata problem would remain.

My reading is that this happens because setting authorized-keys-command and authorized-keys-command-user does not increment available_auth_methods

@samjo-nyang
Copy link
Contributor Author

@willthames I just re-ordered the statements and increase available_auth_methods when use_eic=1. It should work now.

@willthames
Copy link
Contributor

Thanks @samjo-nyang - I'll give it a go!

@willthames
Copy link
Contributor

This works great for me now.

Copy link
Member

@jpculp jpculp left a comment

Choose a reason for hiding this comment

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

This looks great to me!

Also, thank you for testing this out @willthames!

@jpculp jpculp requested a review from zmrow July 9, 2021 17:24
@samjo-nyang
Copy link
Contributor Author

Force-pushed (remove use_eic=1 condition and symlink the host keys for all cases)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants