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

f/instance d/instance: Get (Windows) password data #2219

Merged
merged 4 commits into from
Mar 13, 2018

Conversation

meyertime
Copy link

This is one half of the puzzle which was previously hashicorp/terraform#5675.

This addresses issue hashicorp/terraform#3148 which was moved to #30.

Basically, when an instance is read from AWS (either an instance resource or data source), you now have the option to read the encrypted password data as well. Since not all instances will have password data and it takes time for that data to become available, retrieving this data is optional based on the value of get_password_data, which defaults to false. When true, a password_data attribute is populated with the encrypted data.

This PR is ready to merge, containing updated docs and working acceptance tests. I chose the latest Windows Server 2016 Core image for the tests, since it boots the fastest (and thus the password data becomes available the quickest).

The other half of the puzzle will be decrypting this password data. This will involve implementing built-in functions in Terraform proper to enable this decryption using an interpolated string.

@Ninir Ninir added the enhancement Requests to existing resources that expand the functionality or scope. label Nov 8, 2017
@smastrorocco
Copy link

+1 Would really like to see this merged

@meyertime
Copy link
Author

The second half of this is ready now: hashicorp/terraform#16647

@radeksimko radeksimko added the size/L Managed by automation to categorize the size of a PR. label Nov 15, 2017
@meyertime
Copy link
Author

Can I get a merge here? The necessary rsadecrypt function has been merged. Once this PR is merged and both are released, we can do secure Windows provisioning in AWS like this:

resource "aws_instance" "foo" {
  get_password_data = true
  ...

  connection {
    password = "${rsadecrypt(self.password_data, file("/path/to/private_key.pem"))}"
    ...
  }
}

And/or:

resource "null_resource" "foo" {
  ...

  connection {
    password = "${rsadecrypt(aws_instance.foo.password_data, file("/path/to/private_key.pem"))}"
    ...
  }
}

Or:

data "aws_instance" "foo" {
  get_password_data = true
  ...
}

resource "null_resource" "foo" {
  ...

  connection {
    password = "${rsadecrypt(data.aws_instance.foo.password_data, file("/path/to/private_key.pem"))}"
    ...
  }
}

@jakemig
Copy link

jakemig commented Jan 10, 2018

I'd also love to see this functionality in the AWS provider.

@itsamenathan
Copy link

I would love to see this as well. Looks like the other half of this merge has landed in core as of yesterday.

@grig-tar
Copy link

When you planning to merge this PR?

@jeremygaither
Copy link

I also need this...

@bflad bflad added the service/ec2 Issues and PRs that pertain to the ec2 service. label Jan 28, 2018
Copy link
Contributor

@bflad bflad left a comment

Choose a reason for hiding this comment

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

Hi @deftflux, thanks so much for submitting this (twice!). Its obviously been a long time coming for a PR review 😅 sorry about the lengthy delay. The PR is in actually pretty good shape but I left a few initial review notes. Can you take a look through them, let me know if you have any questions, and when it can be looked at again? 🚀


log.Printf("[INFO] Reading password data for instance %s", d.Id())

timeout := time.Now().Add(15 * time.Minute)
Copy link
Contributor

Choose a reason for hiding this comment

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

Nitpick: we have the resource.Retry function we regularly use for these type of retryable operations:

var passwordData string

input := &ec2.GetPasswordDataInput{
  InstanceId: aws.String(instanceID),
}

err := resource.Retry(15 * time.Minute, func() *resource.RetryError {
  out, err := conn.GetPasswordData(input)

  if err != nil {
    return resource.NonRetryableError(err)
  }

  if resp.PasswordData == nil || *resp.PasswordData == "" {
    return resource.RetryableError(fmt.Errorf("Password data is blank for instance ID: %s", instanceID))
  }

  passwordData = strings.TrimSpace(*resp.PasswordData)
  return nil
})

if err != nil {
  return "", err
}

return passwordData, nil

@@ -1528,6 +1543,46 @@ func readSecurityGroups(d *schema.ResourceData, instance *ec2.Instance, conn *ec
return nil
}

func readPasswordData(d *schema.ResourceData, instance *ec2.Instance, conn *ec2.EC2) error {
Copy link
Contributor

Choose a reason for hiding this comment

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

We should try to reduce this function's signature to one action and leave the state management outside of this function, e.g.

func getAwsEc2InstancePasswordData(instanceID string, conn *ec2.EC2) (string, error)

@@ -1528,6 +1543,46 @@ func readSecurityGroups(d *schema.ResourceData, instance *ec2.Instance, conn *ec
return nil
}

func readPasswordData(d *schema.ResourceData, instance *ec2.Instance, conn *ec2.EC2) error {
if !d.Get("get_password_data").(bool) {
if err := d.Set("password_data", nil); err != nil {
Copy link
Contributor

Choose a reason for hiding this comment

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

When using d.Set() with *string, we do not need the additional err checking. 👍

func testAccInstanceConfig_getPasswordData(val bool) string {
return fmt.Sprintf(`
resource "aws_key_pair" "foo" {
key_name = "tf-acc-winpasswordtest"
Copy link
Contributor

Choose a reason for hiding this comment

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

The key name needs to be randomized for parallel or broken test runs. key_name = "tf-acc-%d" (where %d can come from the top level test function, e.g. rInt := acctest.RandInt()

func testAccInstanceDataSourceConfig_getPasswordData(val bool) string {
return fmt.Sprintf(`
resource "aws_key_pair" "foo" {
key_name = "tf-acc-winpasswordtest"
Copy link
Contributor

Choose a reason for hiding this comment

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

The key name needs to be randomized for parallel or broken test runs. key_name = "tf-acc-%d" (where %d can come from the top level test function, e.g. rInt := acctest.RandInt()

@@ -216,6 +216,84 @@ func TestAccAWSInstanceDataSource_VPCSecurityGroups(t *testing.T) {
})
}

func TestAccAWSInstanceDataSource_getPasswordData_true(t *testing.T) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we reduce these down to two acceptance tests please? true -> false and false -> true should be sufficient

@@ -1288,6 +1288,108 @@ func TestAccAWSInstance_associatePublic_overridePrivate(t *testing.T) {
})
}

func TestAccAWSInstance_getPasswordData_true(t *testing.T) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we reduce this down to two acceptance tests please? false -> true and true -> false should be sufficient

@@ -64,6 +64,7 @@ instance. Amazon defaults this to `stop` for EBS-backed instances and
instances. See [Shutdown Behavior](https://docs.aws.amazon.com/AWSEC2/latest/UserGuide/terminating-instances.html#Using_ChangingInstanceInitiatedShutdownBehavior) for more information.
* `instance_type` - (Required) The type of instance to start
* `key_name` - (Optional) The key name to use for the instance.
* `get_password_data` - (Optional) If true, wait for password data to become available and retrieve it. The password data is exported to the `password_data` attribute.
Copy link
Contributor

Choose a reason for hiding this comment

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

For all mentions, we should specifically add Microsoft Windows in the documentation and potentially link to the API documentation: https://docs.aws.amazon.com/AWSEC2/latest/APIReference/API_GetPasswordData.html

@bflad bflad added the waiting-response Maintainers are waiting on response from community or contributor. label Feb 6, 2018
@joeabbey
Copy link

joeabbey commented Mar 1, 2018

👍 Would love to see this merged

@ghost ghost added the size/L Managed by automation to categorize the size of a PR. label Mar 8, 2018
@meyertime
Copy link
Author

Thanks for the feedback, @bflad. It took a while to get around to making the changes and testing, but it's done now. Please review.

@bflad bflad removed the waiting-response Maintainers are waiting on response from community or contributor. label Mar 8, 2018
@bflad bflad self-requested a review March 8, 2018 15:10
Copy link
Contributor

@bflad bflad left a comment

Choose a reason for hiding this comment

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

This is almost ready! One last thing:

=== RUN   TestAccAWSInstance_importBasic
--- FAIL: TestAccAWSInstance_importBasic (578.44s)
    testing.go:513: Step 1 error: ImportStateVerify attributes not equivalent. Difference is shown below. Top is actual, bottom is expected.
        
        (map[string]string) {
        }
        
        
        (map[string]string) (len=1) {
         (string) (len=17) "get_password_data": (string) (len=5) "false"
        }

=== RUN   TestAccAWSInstance_importInDefaultVpcBySgId
--- FAIL: TestAccAWSInstance_importInDefaultVpcBySgId (290.35s)
    testing.go:513: Step 1 error: ImportStateVerify attributes not equivalent. Difference is shown below. Top is actual, bottom is expected.
        
        (map[string]string) {
        }
        
        
        (map[string]string) (len=1) {
         (string) (len=17) "get_password_data": (string) (len=5) "false"
        }

=== RUN   TestAccAWSInstance_importInEc2Classic
--- FAIL: TestAccAWSInstance_importInEc2Classic (109.72s)
    testing.go:513: Step 1 error: ImportStateVerify attributes not equivalent. Difference is shown below. Top is actual, bottom is expected.
        
        (map[string]string) {
        }
        
        
        (map[string]string) (len=1) {
         (string) (len=17) "get_password_data": (string) (len=5) "false"
        }

It just needs d.Set("get_password_data", ...) called during the read function so importing an instance doesn't show that as a difference.

@bflad bflad added this to the v1.12.0 milestone Mar 9, 2018
@discentem
Copy link

discentem commented Mar 13, 2018

This might be a little off-topic, but does terraform support winrm over https? Tangentially related to this PR, which improves Windows/AWS/Terraform support.

@bflad
Copy link
Contributor

bflad commented Mar 13, 2018

@discentem: There is a https flag for WinRM connections: https://www.terraform.io/docs/provisioners/connection.html#https

@meyertime
Copy link
Author

@discentem a little off-topic, but yes. However, it does require some magic sauce in the user data to set up WinRM over HTTPS on the instance, unless you're using an AMI that already has this set up. The built-in Windows AMI's provided by Amazon do not have WinRM set up at all. Setting up HTTPS also requires generating a certificate within that user data script.

Here's some magic sauce:

<powershell>

echo "Generate certificate for WinRM"
$cert = New-SelfSignedCertificate -CertStoreLocation cert:\LocalMachine\My -DnsName $env:computername
$thumb = $cert.Thumbprint

echo "Enable WinRM"
winrm quickconfig -q
winrm set winrm/config '@{MaxTimeoutms="1800000"}'
winrm set winrm/config/winrs '@{MaxMemoryPerShellMB="300"}'
winrm set winrm/config/service/auth '@{Basic="true"}'
winrm create winrm/config/listener?Address=*+Transport=HTTPS "@{CertificateThumbprint=`"$thumb`";Port=`"5986`"}"

echo "Enable WinRM firewall rules"
if ((get-service -DisplayName "Windows Firewall").StartType -ne "Disabled") {
	Remove-NetFirewallRule -DisplayName "WinRM" -ErrorAction SilentlyContinue
	New-NetFirewallRule -DisplayName "WinRM" -Direction Inbound -Protocol TCP -LocalPort 5986 | Out-Null
}

echo "Restart WinRM service"
Stop-Service winrm
Set-Service winrm -StartupType Automatic
Start-Service winrm

echo "Finished PowerShell script"

</powershell>

Same thing is required when using Packer to build an image. Also, since this uses a self-signed cert, the insecure flag is required as well.

@ghost ghost added the size/L Managed by automation to categorize the size of a PR. label Mar 13, 2018
@meyertime
Copy link
Author

@bflad Not sure how to bounce this back to you, but I made the requested change. The tests should pass now.

@bflad bflad self-requested a review March 13, 2018 17:09
Copy link
Contributor

@bflad bflad 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 -- thanks so much for this contribution! 🎉 🚀

 61 tests passed (all tests)
=== RUN   TestAccAWSInstanceDataSource_keyPair
--- PASS: TestAccAWSInstanceDataSource_keyPair (87.12s)
=== RUN   TestAccAWSInstanceDataSource_SecurityGroups
--- PASS: TestAccAWSInstanceDataSource_SecurityGroups (182.30s)
=== RUN   TestAccAWSInstance_importInDefaultVpcBySgName
--- PASS: TestAccAWSInstance_importInDefaultVpcBySgName (205.71s)
=== RUN   TestAccAWSInstanceDataSource_rootInstanceStore
--- PASS: TestAccAWSInstanceDataSource_rootInstanceStore (251.71s)
=== RUN   TestAccAWSInstanceDataSource_privateIP
--- PASS: TestAccAWSInstanceDataSource_privateIP (255.15s)
=== RUN   TestAccAWSInstancesDataSource_basic
--- PASS: TestAccAWSInstancesDataSource_basic (277.11s)
=== RUN   TestAccAWSInstance_importInDefaultVpcBySgId
--- PASS: TestAccAWSInstance_importInDefaultVpcBySgId (279.31s)
=== RUN   TestAccAWSInstancesDataSource_tags
--- PASS: TestAccAWSInstancesDataSource_tags (310.58s)
=== RUN   TestAccAWSInstanceDataSource_PlacementGroup
--- PASS: TestAccAWSInstanceDataSource_PlacementGroup (320.15s)
=== RUN   TestAccAWSInstance_GP2IopsDevice
--- PASS: TestAccAWSInstance_GP2IopsDevice (134.02s)
=== RUN   TestAccAWSInstanceDataSource_VPCSecurityGroups
--- PASS: TestAccAWSInstanceDataSource_VPCSecurityGroups (361.57s)
=== RUN   TestAccAWSInstance_importBasic
--- PASS: TestAccAWSInstance_importBasic (369.00s)
=== RUN   TestAccAWSInstance_blockDevices
--- PASS: TestAccAWSInstance_blockDevices (120.83s)
=== RUN   TestAccAWSInstanceDataSource_gp2IopsDevice
--- PASS: TestAccAWSInstanceDataSource_gp2IopsDevice (383.42s)
=== RUN   TestAccAWSInstance_noAMIEphemeralDevices
--- PASS: TestAccAWSInstance_noAMIEphemeralDevices (113.28s)
=== RUN   TestAccAWSInstance_basic
--- PASS: TestAccAWSInstance_basic (305.76s)
=== RUN   TestAccAWSInstanceDataSource_tags
--- PASS: TestAccAWSInstanceDataSource_tags (407.71s)
=== RUN   TestAccAWSInstanceDataSource_basic
--- PASS: TestAccAWSInstanceDataSource_basic (417.96s)
=== RUN   TestAccAWSInstance_userDataBase64
--- PASS: TestAccAWSInstance_userDataBase64 (262.84s)
=== RUN   TestAccAWSInstance_ipv6AddressCountAndSingleAddressCausesError
--- PASS: TestAccAWSInstance_ipv6AddressCountAndSingleAddressCausesError (127.04s)
=== RUN   TestAccAWSInstance_placementGroup
--- PASS: TestAccAWSInstance_placementGroup (176.61s)
=== RUN   TestAccAWSInstance_vpc
--- PASS: TestAccAWSInstance_vpc (205.03s)
=== RUN   TestAccAWSInstanceDataSource_getPasswordData_falseToTrue
--- PASS: TestAccAWSInstanceDataSource_getPasswordData_falseToTrue (594.24s)
=== RUN   TestAccAWSInstance_rootInstanceStore
--- PASS: TestAccAWSInstance_rootInstanceStore (346.69s)
=== RUN   TestAccAWSInstanceDataSource_AzUserData
--- PASS: TestAccAWSInstanceDataSource_AzUserData (631.28s)
=== RUN   TestAccAWSInstanceDataSource_blockDevices
--- PASS: TestAccAWSInstanceDataSource_blockDevices (639.29s)
=== RUN   TestAccAWSInstanceDataSource_getPasswordData_trueToFalse
--- PASS: TestAccAWSInstanceDataSource_getPasswordData_trueToFalse (641.53s)
=== RUN   TestAccAWSInstance_multipleRegions
--- PASS: TestAccAWSInstance_multipleRegions (252.68s)
=== RUN   TestAccAWSInstance_tags
--- PASS: TestAccAWSInstance_tags (215.91s)
=== RUN   TestAccAWSInstance_NetworkInstanceVPCSecurityGroupIDs
--- PASS: TestAccAWSInstance_NetworkInstanceVPCSecurityGroupIDs (251.93s)
=== RUN   TestAccAWSInstance_volumeTagsComputed
--- PASS: TestAccAWSInstance_volumeTagsComputed (144.39s)
=== RUN   TestAccAWSInstance_instanceProfileChange
--- PASS: TestAccAWSInstance_instanceProfileChange (197.58s)
=== RUN   TestAccAWSInstance_NetworkInstanceRemovingAllSecurityGroups
--- PASS: TestAccAWSInstance_NetworkInstanceRemovingAllSecurityGroups (340.99s)
=== RUN   TestAccAWSInstance_keyPairCheck
--- PASS: TestAccAWSInstance_keyPairCheck (110.85s)
=== RUN   TestAccAWSInstance_sourceDestCheck
--- PASS: TestAccAWSInstance_sourceDestCheck (457.59s)
=== RUN   TestAccAWSInstance_withIamInstanceProfile
--- PASS: TestAccAWSInstance_withIamInstanceProfile (238.95s)
=== RUN   TestAccAWSInstance_primaryNetworkInterface
--- PASS: TestAccAWSInstance_primaryNetworkInterface (214.25s)
=== RUN   TestAccAWSInstance_GP2WithIopsValue
--- PASS: TestAccAWSInstance_GP2WithIopsValue (650.03s)
=== RUN   TestAccAWSInstance_associatePublicIPAndPrivateIP
--- PASS: TestAccAWSInstance_associatePublicIPAndPrivateIP (301.50s)
=== RUN   TestAccAWSInstance_disableApiTermination
--- PASS: TestAccAWSInstance_disableApiTermination (618.64s)
=== RUN   TestAccAWSInstanceDataSource_VPC
--- PASS: TestAccAWSInstanceDataSource_VPC (962.61s)
=== RUN   TestAccAWSInstance_associatePublic_defaultPublic
--- PASS: TestAccAWSInstance_associatePublic_defaultPublic (198.08s)
=== RUN   TestAccAWSInstance_ipv6_supportAddressCountWithIpv4
--- PASS: TestAccAWSInstance_ipv6_supportAddressCountWithIpv4 (587.44s)
=== RUN   TestAccAWSInstance_primaryNetworkInterfaceSourceDestCheck
--- PASS: TestAccAWSInstance_primaryNetworkInterfaceSourceDestCheck (292.45s)
=== RUN   TestAccAWSInstance_rootBlockDeviceMismatch
--- PASS: TestAccAWSInstance_rootBlockDeviceMismatch (371.37s)
=== RUN   TestAccAWSInstance_associatePublic_explicitPublic
--- PASS: TestAccAWSInstance_associatePublic_explicitPublic (184.21s)
=== RUN   TestAccAWSInstance_volumeTags
--- PASS: TestAccAWSInstance_volumeTags (545.44s)
=== RUN   TestAccAWSInstance_forceNewAndTagsDrift
--- PASS: TestAccAWSInstance_forceNewAndTagsDrift (432.28s)
=== RUN   TestAccAWSInstance_associatePublic_explicitPrivate
--- PASS: TestAccAWSInstance_associatePublic_explicitPrivate (216.12s)
=== RUN   TestAccAWSInstance_associatePublic_overridePublic
--- PASS: TestAccAWSInstance_associatePublic_overridePublic (204.65s)
=== RUN   TestAccAWSInstance_associatePublic_overridePrivate
--- PASS: TestAccAWSInstance_associatePublic_overridePrivate (185.09s)
=== RUN   TestAccAWSInstance_ipv6_supportAddressCount
--- PASS: TestAccAWSInstance_ipv6_supportAddressCount (800.62s)
=== RUN   TestAccAWSInstance_NetworkInstanceSecurityGroups
--- PASS: TestAccAWSInstance_NetworkInstanceSecurityGroups (789.77s)
=== RUN   TestAccAWSInstance_privateIP
--- PASS: TestAccAWSInstance_privateIP (564.16s)
=== RUN   TestAccAWSInstance_associatePublic_defaultPrivate
--- PASS: TestAccAWSInstance_associatePublic_defaultPrivate (462.32s)
=== RUN   TestAccAWSInstance_addSecurityGroupNetworkInterface
--- PASS: TestAccAWSInstance_addSecurityGroupNetworkInterface (466.05s)
=== RUN   TestAccAWSInstance_getPasswordData_falseToTrue
--- PASS: TestAccAWSInstance_getPasswordData_falseToTrue (355.86s)
=== RUN   TestAccAWSInstance_importInEc2Classic
--- PASS: TestAccAWSInstance_importInEc2Classic (1345.27s)
=== RUN   TestAccAWSInstance_changeInstanceType
--- PASS: TestAccAWSInstance_changeInstanceType (698.59s)
=== RUN   TestAccAWSInstance_addSecondaryInterface
--- PASS: TestAccAWSInstance_addSecondaryInterface (618.78s)
=== RUN   TestAccAWSInstance_getPasswordData_trueToFalse
--- PASS: TestAccAWSInstance_getPasswordData_trueToFalse (573.26s)

@bflad bflad merged commit 8d6c848 into hashicorp:master Mar 13, 2018
bflad added a commit that referenced this pull request Mar 13, 2018
@meyertime
Copy link
Author

👍 🎉

@jeremygaither
Copy link

@deftflux THANK YOU!

🎉🎉🎉🎉🎉🎉🎉🎉🎉🎉🎉🎉

@discentem
Copy link

discentem commented Mar 18, 2018

Anyone willing to post instructions on overriding the released aws provider with a build that includes these changes? I built it, dropped it into ~/.terraform.d/plugins/, ran terraform init (no changes), but terraform still doesn't recognize get_password_data.

@discentem
Copy link

I pieced it together. A bit complicated though. It seems terraform doesn't respect a higher z value when parsing plugin_vx.y.z. Had to bump y by 1.

@bflad
Copy link
Contributor

bflad commented Mar 23, 2018

This has been released in version 1.12.0 of the AWS provider. Please see the Terraform documentation on provider versioning or reach out if you need any assistance upgrading.

@discentem
Copy link

Thanks @bflad. I found the docs for specifying the version in the provider code 👍

@bodgit
Copy link
Contributor

bodgit commented Apr 12, 2018

I don't think this was implemented for spot requests so it's breaks if you try and spin up a Windows instance as a spot request and try and access the password data. I'll see about creating a PR to fix this.

@midacts
Copy link

midacts commented Apr 11, 2019

Does anyone know if this is documented anywhere? Maybe in the aws_instance resource docs?
The solution to this problem was not easy to find.

@ghost
Copy link

ghost commented Mar 30, 2020

I'm going to lock this issue because it has been closed for 30 days ⏳. This helps our maintainers find and focus on the active issues.

If you feel this issue should be reopened, we encourage creating a new issue linking back to this one for added context. Thanks!

@ghost ghost locked and limited conversation to collaborators Mar 30, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
enhancement Requests to existing resources that expand the functionality or scope. service/ec2 Issues and PRs that pertain to the ec2 service. size/L Managed by automation to categorize the size of a PR.
Projects
None yet
Development

Successfully merging this pull request may close these issues.