hashicorp / packer-plugin-amazon

Packer plugin for Amazon AMI Builder
https://www.packer.io/docs/builders/amazon
Mozilla Public License 2.0
76 stars 112 forks source link

session_manager: Upload user public key to instance before connection if private key is specified #296

Closed Glyphack closed 1 year ago

Glyphack commented 2 years ago

Improves SSM session manager feature by disabling key pair creation when user specifies a private key pair to be used.

The current implementation works fine for cases where the user does not specify a private key to be used with session_manager. In cases where the user specifies a private key, then that key will be used to authenticate but it fails since the key is not present on the EC2 instance.

This PR will resolve the issue by sending the user public key to the instance before connecting to it.

The New Behavior

If the user does not specify a private key, then works like the previous implementations.

In the case where the user specifies a private key to be used:

  1. key pair will not be created.
  2. the public key will be sent to the instance before connecting to it. This key will live for 60 seconds which is enough for the handshake.

Questions

There are some limitations to what public keys we can send to instances with SSM. The SendSSHPublicKeyInput struct in aws-sdk-go currently used in the project only supports RSA keys because it has a limit on the length of the public key. This behavior is fixed in the recent version so if we update it then it supports other keys.

How can I update the docs?

I'll complete the code and add it to other builders once I get initial feedback on the implementation.

Closes #31

hashicorp-cla commented 2 years ago

CLA assistant check
All committers have signed the CLA.

Glyphack commented 1 year ago

@nywilken Since you implemented this feature originally, can you give me some feedback on this approach?

lbajolet-hashicorp commented 1 year ago

Oh also regarding the minimum size for the SSHPublicKey in the SDK, we can probably bump the version we rely on to support the 80 bytes minimum, this way we won't reject ED25519 (and probably other) keys for this operation.

Judging by their docs, this changed in v1.42.37, so we can bump to this version and we should support those keys then.

Glyphack commented 1 year ago

Thanks for the review @lbajolet-hashicorp. I resolved the comments, but I would appreciate your input on the remaining topics: https://github.com/hashicorp/packer-plugin-amazon/pull/296#discussion_r1053373320

Also I'm not familiar with how the acceptance tests work, could you help me with how to write one? This is the configuration required to test this bugfix:

source "amazon-ebs" "hvr" {
  ami_name      = "test-${formatdate("YYYY-MM-DD-hhmmss", timestamp())}"
  instance_type = "t2.micro"
  region        = "eu-west-1"
  source_ami_filter {
    filters = {
      name                = "amzn2-ami-hvm-*-x86_64-ebs"
      root-device-type    = "ebs"
      virtualization-type = "hvm"
    }
    most_recent = true
    owners      = ["amazon"]
  }
  ssh_username  = "ec2-user"
  ssh_interface = "session_manager"
  communicator  = "ssh"
  temporary_iam_instance_profile_policy_document {
    Statement {
      Action   = ["sts:AssumeRole"]
      Effect   = "Allow"
      Resource = ["*"]
      Principal = {
        "Service" : "ec2.amazonaws.com"
      }
    }
    Version = "2012-10-17"
  }
  ssh_private_key_file = "~/.ssh/id_rsa"
  vpc_id               = "vpc-0d2aa27cfe71c1bc4"
  subnet_id            = "subnet-0e4b7f6537867c07b"
}

build {
  name = "hvr"
  sources = [
    "source.amazon-ebs.hvr"
  ]
}
lbajolet-hashicorp commented 1 year ago

Hi @Glyphack,

Just wanted to ping on this PR, do you need assistance on landing this? I can probably set some time to work on it next week if you need help, let me know.

Glyphack commented 1 year ago

Hey @lbajolet-hashicorp thanks The last comment is clear to me; I'm going to apply it. I can get some help with writing the acceptance test in case it's needed.

lbajolet-hashicorp commented 1 year ago

Hi @Glyphack,

Sure thing, when you're done with the latest reroll, let me know, and I'll take a jab at the acceptance testing.

For reference, acceptance tests are written for ec2 and ebssurrogate for now (SEE: https://github.com/hashicorp/packer-plugin-amazon/blob/main/builder/ebs/builder_acc_test.go for example).

When we're ready to test the feature, I'll get to work on that.

Thanks again!

Glyphack commented 1 year ago

Hi @lbajolet-hashicorp

This is ready for review and test(I did it locally). Just a few notes/questions This is the final packer file I used to test the functionality. It is based on the original issue:

source "amazon-ebs" "test" {
  ami_name      = "test-${formatdate("YYYY-MM-DD-hhmmss", timestamp())}"
  instance_type = "t2.micro"
  region        = "eu-west-1"
  source_ami_filter {
    filters = {
      name                = "amzn2-ami-hvm-*-x86_64-ebs"
      root-device-type    = "ebs"
      virtualization-type = "hvm"
    }
    most_recent = true
    owners      = ["amazon"]
  }
  ssh_username  = "ec2-user"
  ssh_interface = "session_manager"
  communicator  = "ssh"
  /* This is just a profile with SSMManagedInstanceCore policy attached */
  iam_instance_profile = "packer_profile"
  /* This is my private key which is used for all ssh connections by default */
  ssh_private_key_file = "~/.ssh/id_ed25519"

I have also added this to other builders as well(initially it was only working with ebs). I haven't used the other builders so if something is not compatible with this change let me know.

lbajolet-hashicorp commented 1 year ago

Oh, there are some conflicts also to fix, I'd suggest rebasing this PR on top of the current main, fixing the conflicts, and pushing the branch again so we're clear to merge

Glyphack commented 1 year ago

@lbajolet-hashicorp Thank you so much for the effort.

I rebased the branch and applied the suggestions. Note I removed the AWS SDK version bump from changes since it's already updated in the main.

lbajolet-hashicorp commented 1 year ago

@Glyphack thanks to you for the rerolls and the preseverance!

I took the liberty of pushing a few acceptance tests for both the ebs and the ebssurrogate builders for this feature, I'll see that someone else on the team reviews your PR one last time to make sure I didn't introduce something that I shouldn't, but after that, we can merge it.

Thanks again for the collaboration, much appreciated