Add tpm support #379

alemuro · 2023-05-11T17:19:13Z

Hello! 👋

I'm interested in merging the PR #339 but it seems there is no activity since March. For that reason I opened a this PR with changes (and history commits) from the other PR + addressing the comments from that PR and rebasing the branch against the main branch.

The author says on its MR:

Adding AWS NitroTPM support to AMI. More information can be found here:
https://docs.aws.amazon.com/AWSEC2/latest/UserGuide/enable-nitrotpm-support-on-ami.html
https://docs.aws.amazon.com/AWSEC2/latest/UserGuide/enable-nitrotpm-prerequisites.html

There is a flag that is not currently supported by the Packer AMI Builder: --tpm-support v2.0

This update is meant to resolve that.

I'm also interested in adding this feature to the ebs builder. I'm not sure if this is a good opportunity to change the ebs builder to use the Register API, or if I should raise another PR for that use case. What do you guys think?

Tests

I've run go build and make test and seems to work with no issues. I also tried make testacc

$ make test
?       github.com/hashicorp/packer-plugin-amazon       [no test files]
ok      github.com/hashicorp/packer-plugin-amazon/builder/chroot        2.006s
ok      github.com/hashicorp/packer-plugin-amazon/builder/common        0.933s
?       github.com/hashicorp/packer-plugin-amazon/builder/common/awserrors      [no test files]
?       github.com/hashicorp/packer-plugin-amazon/builder/common/ssm    [no test files]
ok      github.com/hashicorp/packer-plugin-amazon/builder/ebs   8.888s
?       github.com/hashicorp/packer-plugin-amazon/builder/ebs/acceptance        [no test files]
ok      github.com/hashicorp/packer-plugin-amazon/builder/ebssurrogate  3.101s
ok      github.com/hashicorp/packer-plugin-amazon/builder/ebsvolume     2.366s
ok      github.com/hashicorp/packer-plugin-amazon/builder/instance      2.629s
ok      github.com/hashicorp/packer-plugin-amazon/datasource/ami        0.908s
ok      github.com/hashicorp/packer-plugin-amazon/datasource/parameterstore     1.496s
ok      github.com/hashicorp/packer-plugin-amazon/datasource/secretsmanager     1.784s
?       github.com/hashicorp/packer-plugin-amazon/post-processor/import [no test files]
?       github.com/hashicorp/packer-plugin-amazon/version       [no test files]

Closes #314

…zon/blob/main/.github/CONTRIBUTING.md#opening-an-pull-request

hashicorp-cla · 2023-05-11T17:19:18Z

All committers have signed the CLA.

lbajolet-hashicorp · 2023-05-11T20:21:39Z

Hey @alemuro,

Regarding the ebs builder, we'd need to change how the AMI is created, so while I'm not against the idea of providing an alternative way to do this rather than using CreateImage, I think it should be optional. From my understanding the only way we could do this is to first snapshot the root volume, and use RegisterImage to make an AMI from this snapshot.

Adding this to the builder would indeed make it possible to use this workflow and benefit from the extra options that RegisterImage provides, but if possible I would like to wait until we make a move on this front, at least until we have a way to run our acceptance tests reliably from a Github action, that way we can be more confident this won't break existing configs.

Alternatively, and this is more of a gamble, we could wait until there's another way to set this attribute on existing AMIs, much like what we experienced with the IMDSv2 enforcement earlier this year. This would be preferable in my opinion due to the lesser risk to the builder, which is our most popular for this plugin, hence my reluctance in changing its workflow.

On another note, thanks for picking this PR up! I'll do another review pass now, and if it looks good, we can merge it.

lbajolet-hashicorp

Pre-approving in the expectative for a reroll with my comments addressed, but overall this looks good to me. The only thing I'd maybe like to see would be an acceptance test for ebssurrogate, but as it stands I think it's good already, we can do another pass to make sure this works as intended.

Thanks for picking this up again @alemuro !

lbajolet-hashicorp · 2023-05-11T20:23:33Z

docs-partials/builder/common/BlockDevice.mdx

@@ -16,6 +16,7 @@ launch_block_device_mappings {
    encrypted = true
    kms_key_id = "1a2b3c4d-5e6f-1a2b-3c4d-5e6f1a2b3c4d"
 }
+


I wonder why this changed, is it something that happens when you run make generate? If so we're good, I'm more curious than anything

Yup! I didn't changed the file manually but with make generate.

lbajolet-hashicorp · 2023-05-11T20:26:25Z

builder/chroot/builder.go

@@ -376,6 +380,10 @@ func (b *Builder) Prepare(raws ...interface{}) ([]string, []string, error) {
 		errs = packersdk.MultiErrorAppend(errs, errors.New(`The only valid ami_architecture values are "arm64", "i386", "x86_64", or "x86_64_mac"`))
 	}

+	if b.config.TpmSupport != "" && b.config.TpmSupport != ec2.TpmSupportValuesV20 {
+		errs = packersdk.MultiErrorAppend(errs, fmt.Errorf(`The only valid tpm_support values are %q or the empty string`, ec2.TpmSupportValuesV20))


Little nit that I think I forgot to highlight in my first review, the or the empty string part feels off to me, the empty string is the default value, which is equivalent to not specifying it, so I think we can rework this message to give out the only acceptable value.

Suggested change

errs = packersdk.MultiErrorAppend(errs, fmt.Errorf(`The only valid tpm_support values are %q or the empty string`, ec2.TpmSupportValuesV20))

errs = packersdk.MultiErrorAppend(errs, fmt.Errorf(`The only valid tpm_support value is %q`, ec2.TpmSupportValuesV20))

This would apply to all the builders that support the feature.

Make sense, added to the three builders where this PR applies: chroot, ebssurrogate, and instance.
Thanks!

alemuro · 2023-05-12T06:52:51Z

Morning @lbajolet-hashicorp ,

I've added tests to the other builders as requested, the make test command runs with no failure:

$ make test
?       github.com/hashicorp/packer-plugin-amazon       [no test files]
ok      github.com/hashicorp/packer-plugin-amazon/builder/chroot        1.088s
ok      github.com/hashicorp/packer-plugin-amazon/builder/common        2.295s
?       github.com/hashicorp/packer-plugin-amazon/builder/common/awserrors      [no test files]
?       github.com/hashicorp/packer-plugin-amazon/builder/common/ssm    [no test files]
ok      github.com/hashicorp/packer-plugin-amazon/builder/ebs   6.472s
?       github.com/hashicorp/packer-plugin-amazon/builder/ebs/acceptance        [no test files]
ok      github.com/hashicorp/packer-plugin-amazon/builder/ebssurrogate  1.100s
ok      github.com/hashicorp/packer-plugin-amazon/builder/ebsvolume     2.581s
ok      github.com/hashicorp/packer-plugin-amazon/builder/instance      2.985s
ok      github.com/hashicorp/packer-plugin-amazon/datasource/ami        0.880s
ok      github.com/hashicorp/packer-plugin-amazon/datasource/parameterstore     2.580s
ok      github.com/hashicorp/packer-plugin-amazon/datasource/secretsmanager     3.102s
?       github.com/hashicorp/packer-plugin-amazon/post-processor/import [no test files]
?       github.com/hashicorp/packer-plugin-amazon/version       [no test files]

Regarding the ebs builder, I agree 100% with you.

Thanks for your revision!

alemuro · 2023-05-12T10:57:04Z

Ok I saw the BlockDevice.mdx thing. I thought this was generated by the make generate, but seems I was completely wrong... I run it again and now the newline has been removed. Apologies 🙏

Now it should be fine.

lbajolet-hashicorp · 2023-05-12T14:12:33Z

Hi @alemuro,

Thanks for being so quick at integrating my comments, much appreciated!

With this reroll, it all looks good to me, I'm merging this now.

alemuro · 2023-05-24T08:05:08Z

Hello @lbajolet-hashicorp , do you have a tentative date on when this PR will be released on a new release? Thanks!!

lbajolet-hashicorp · 2023-05-24T13:33:35Z

Hi @alemuro,

Considering it's been a few weeks since the last release, and from what I see we don't have a ton on the table on this plugin now, we can probably release a new version soon, in the coming days or maybe next week sounds feasible I would think!

alemuro · 2023-05-24T13:42:43Z

Cool thanks!

ccravens added 2 commits May 11, 2023 18:52

Added TPM as configuration option

84c14f8

Go format according to https://github.com/hashicorp/packer-plugin-ama…

88a3760

…zon/blob/main/.github/CONTRIBUTING.md#opening-an-pull-request

alemuro requested a review from a team as a code owner May 11, 2023 17:19

lbajolet-hashicorp approved these changes May 11, 2023

View reviewed changes

alemuro force-pushed the add-tpm branch 2 times, most recently from 5361101 to 7da7c1f Compare May 12, 2023 06:50

Address PR comments

4791abc

alemuro force-pushed the add-tpm branch from 7da7c1f to 4791abc Compare May 12, 2023 10:56

lbajolet-hashicorp merged commit 1b68690 into hashicorp:main May 12, 2023

glachac-safelishare mentioned this pull request May 23, 2023

Add configuration to enable NitroTPM support when registering an AMI #314

Open

lbajolet-hashicorp mentioned this pull request May 24, 2023

Tpm support #339

Closed

lbajolet-hashicorp added the enhancement label Jun 5, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add tpm support #379

Add tpm support #379

alemuro commented May 11, 2023

hashicorp-cla commented May 11, 2023 •

edited

Loading

lbajolet-hashicorp commented May 11, 2023

lbajolet-hashicorp left a comment

lbajolet-hashicorp May 11, 2023

alemuro May 12, 2023

alemuro May 12, 2023

lbajolet-hashicorp May 11, 2023

lbajolet-hashicorp May 11, 2023

alemuro May 12, 2023 •

edited

Loading

alemuro commented May 12, 2023

alemuro commented May 12, 2023

lbajolet-hashicorp commented May 12, 2023

alemuro commented May 24, 2023

lbajolet-hashicorp commented May 24, 2023

alemuro commented May 24, 2023

	errs = packersdk.MultiErrorAppend(errs, fmt.Errorf(`The only valid tpm_support values are %q or the empty string`, ec2.TpmSupportValuesV20))
	errs = packersdk.MultiErrorAppend(errs, fmt.Errorf(`The only valid tpm_support value is %q`, ec2.TpmSupportValuesV20))

Add tpm support #379

Add tpm support #379

Conversation

alemuro commented May 11, 2023

Tests

hashicorp-cla commented May 11, 2023 • edited Loading

lbajolet-hashicorp commented May 11, 2023

lbajolet-hashicorp left a comment

Choose a reason for hiding this comment

lbajolet-hashicorp May 11, 2023

Choose a reason for hiding this comment

alemuro May 12, 2023

Choose a reason for hiding this comment

alemuro May 12, 2023

Choose a reason for hiding this comment

lbajolet-hashicorp May 11, 2023

Choose a reason for hiding this comment

lbajolet-hashicorp May 11, 2023

Choose a reason for hiding this comment

alemuro May 12, 2023 • edited Loading

Choose a reason for hiding this comment

alemuro commented May 12, 2023

alemuro commented May 12, 2023

lbajolet-hashicorp commented May 12, 2023

alemuro commented May 24, 2023

lbajolet-hashicorp commented May 24, 2023

alemuro commented May 24, 2023

hashicorp-cla commented May 11, 2023 •

edited

Loading

alemuro May 12, 2023 •

edited

Loading