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.

Sign up for GitHub

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

Jump to bottom

✨ Ignition: add option to store User Data in plain text #4700

Merged
merged 2 commits into from
Feb 13, 2024
Merged

✨ Ignition: add option to store User Data in plain text #4700

merged 2 commits into from
Feb 13, 2024

Conversation

damdo
Copy link
Member

@damdo damdo commented Dec 18, 2023 •
edited
Loading

What type of PR is this?
/kind feature

What this PR does / why we need it:

For Machine bootstrapping CAPA supports one main method: CloudInit and one experimental method: Ignition.

For the former, CAPA used to store the User Data that the bootstrapper would use to fulfill its job in the EC2 Instance User Data in plain text.

This was not considered the most secure way to provide this configuration to a machine as a process/subject with the power to query the instance metadata could be able to read this config, which may contain certs, keys and other forms of secrets.

As such to minimize this issue, a new default method was added, where the controller would now leverage AWS SSM to encrypt the user data before storing it in the EC2 Instance User Data. The previous method of storing User Data in plain text was kept available as an alternative option to users, which would be able to access it by providing the InsecureSkipSecretsManager option in the AWSMachine spec.

Later on, when the new Ignition bootstrapping support was added, the creators, already aware of the security implications of storing User Data in plain text,
decided to opt for a more secure default. After briefly looking at using AWS SSM encryption also for Ignition, they acknowledged that it wasn't possible due to Ignition's lack of support for multi mime types. Hence they went for using S3 Buckets to store the config in objects, and have ignition fetch it at bootstrap time from there, and this did the trick.

Up until now this has been the only option to achieve Ignition bootstrapping.
However even if this is a better option from a security standpoint than storing User Data in plain text, and as such the preferred option for a good default,
it requires the ability to provision/access S3 Buckets, which is not always possible/desirable in specific scenarios, as well as not suitable for certain type of bespoke CAPI bootstrap providers.

As such we would like to introduce for Ignition, similarly to what is available for CloudInit, the ability to store User Data in the EC2 Instance in plain text.
Of course we know this is not the best option from a security standpoint, and as such we want to make it a fallback option that users can leverage if their use case does not allow the use of S3 buckets. We mentioned in the docs, and in the API that this option is discouraged and we trust CAPA users will use this with care.

Special notes for your reviewer:

Checklist:

  • squashed commits
  • includes documentation
  • includes emojis
  • adds unit tests

Release note:

Ignition: add option to store User Data in plain text

@k8s-ci-robot k8s-ci-robot added release-note Denotes a PR that will be considered when it comes time to generate release notes. kind/feature Categorizes issue or PR as related to a new feature. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. labels Dec 18, 2023
@k8s-ci-robot k8s-ci-robot added needs-priority size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. labels Dec 18, 2023
@damdo
Copy link
Member Author

damdo commented Dec 18, 2023

/cc @JoelSpeed @vincepri @nrb

JoelSpeed
JoelSpeed reviewed Dec 18, 2023
View reviewed changes
Copy link

@JoelSpeed JoelSpeed left a comment

Choose a reason for hiding this comment

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

We should discuss with other maintainers before making any changes, because the code here LGTM without any modification.

However one thought occurred that I thought I would raise. The current mode, providing no modifications, is almost like a passthrough mode. I'm wondering if passthrough may be a better way to describe it, as users may have got their own system for encryption or whatever/things may evolve in the future to allow future expansion. Having a default that is passthrough, makes it open to any future modification since it's clear that the content will just be passed through to the metadata without any modification.

Could be more future proof than PlainTextUserData

@damdo
Copy link
Member Author

damdo commented Dec 21, 2023

/test pull-cluster-api-provider-aws-test

@dlipovetsky
Copy link
Contributor

If the user is selecting a storage mechanism, then I think the two choices are:

  • S3
  • Instance Userdata

https://docs.aws.amazon.com/AWSEC2/latest/UserGuide/instancedata-add-user-data.html

So I think InstanceUserdata would be a good name.

@richardcase
Copy link
Member

/milestone v2.4.0

@k8s-ci-robot k8s-ci-robot added this to the v2.4.0 milestone Jan 8, 2024
Ankitasw
Ankitasw reviewed Jan 11, 2024
View reviewed changes
Copy link
Member

@Ankitasw Ankitasw left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Agree with what @dlipovetsky suggested.
This PR overall seems ok to me if we have a valid use case around this.
Overall code is also good, some small nits/questions from my end.

docs/book/src/topics/ignition-support.md Show resolved Hide resolved
docs/book/src/topics/ignition-support.md Show resolved Hide resolved
docs/book/src/topics/ignition-support.md Outdated Show resolved Hide resolved
@damdo
Copy link
Member Author

damdo commented Jan 11, 2024

@dlipovetsky @richardcase as discussed during the CAPA meeting I changed the storageType value from PlainTextUserData to InstanceUserData (cc. @JoelSpeed)

@Ankitasw I've addressed your comments 👍

Thanks all for the input here.

@damdo damdo requested a review from Ankitasw January 11, 2024 15:18
@damdo damdo mentioned this pull request Jan 16, 2024
Merged
@k8s-ci-robot k8s-ci-robot added needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. and removed needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. labels Jan 20, 2024
@damdo damdo requested a review from vincepri January 23, 2024 11:49
@richardcase richardcase removed this from the v2.4.0 milestone Jan 23, 2024
@richardcase
Copy link
Member

/milestone v2.4.0

@k8s-ci-robot k8s-ci-robot added this to the v2.4.0 milestone Jan 23, 2024
vincepri
vincepri approved these changes Jan 23, 2024
View reviewed changes
Copy link
Member

@vincepri vincepri left a comment

Choose a reason for hiding this comment

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

/lgtm

@damdo
Copy link
Member Author

damdo commented Feb 6, 2024

/test pull-cluster-api-provider-aws-test

@damdo
Copy link
Member Author

damdo commented Feb 6, 2024

/test pull-cluster-api-provider-aws-e2e

@richardcase
Copy link
Member

I'm seeing this in the CAPA logs for the failing e2e:

E0206 11:48:06.531883       1 awsmachine_controller.go:519] "unable to create instance" err="failed to resolve userdata: unsupported ignition storageType \"\""
E0206 11:48:06.532561       1 controller.go:329] "Reconciler error" err="failed to resolve userdata: unsupported ignition storageType \"\"" controller="awsmachine" controllerGroup="infrastructure.cluster.x-k8s.io" controllerKind="AWSMachine" AWSMachine="functional-test-ignition-w6200e/functional-test-ignition-hqv6ar-control-plane-bqqxz" namespace="functional-test-ignition-w6200e" name="functional-test-ignition-hqv6ar-control-plane-bqqxz" reconcileID="5d081c07-ba4e-40c1-8eb7-b4d3aa441a37"

@damdo
Copy link
Member Author

damdo commented Feb 6, 2024

/test pull-cluster-api-provider-aws-e2e

@nrb
Copy link
Contributor

nrb commented Feb 6, 2024 •
edited
Loading

/test pull-cluster-api-provider-aws-e2e
time out
Logs show that no NAT Gateway was availabe for ~30 minutes.

@vincepri
Copy link
Member

vincepri commented Feb 6, 2024

/test pull-cluster-api-provider-aws-e2e

2 similar comments
@damdo
Copy link
Member Author

damdo commented Feb 7, 2024

/test pull-cluster-api-provider-aws-e2e

@damdo
Copy link
Member Author

damdo commented Feb 7, 2024

/test pull-cluster-api-provider-aws-e2e

@damdo
Copy link
Member Author

damdo commented Feb 7, 2024

Found the issue with the deprovisioning, let's see if E2Es pass now:

/test pull-cluster-api-provider-aws-e2e

@damdo
Copy link
Member Author

damdo commented Feb 7, 2024

@vincepri @nrb @richardcase "old" e2e passed 🎉
I'll re-add the new e2e case for unencrypted UserData, and see if that works.

@damdo
ignition: add option to store User Data in plain text
6f56ed2 
ignition: run make generate
ignition: add storageType implementation + adapt existing tests
ignition/cloudinit: improve testing structures
ignition: update docs
@damdo
Copy link
Member Author

damdo commented Feb 9, 2024

/test pull-cluster-api-provider-aws-e2e

@vincepri
Copy link
Member

vincepri commented Feb 9, 2024

Seems another test is failing:

STEP: Waiting for the workload nodes to exist - /home/prow/go/pkg/mod/sigs.k8s.io/cluster-api/[email protected]/framework/machinedeployment_helpers.go:106 @ 02/08/24 15:22:47.926
[FAILED] Expected
    <string>: cloud-config
to equal
    <string>: ignition
In [It] at: /home/prow/go/src/sigs.k8s.io/cluster-api-provider-aws/test/e2e/suites/unmanaged/unmanaged_functional_test.go:1105 @ 02/08/24 15:23:48.067

@damdo
Copy link
Member Author

damdo commented Feb 9, 2024

Seems another test is failing:

@vincepri
Thanks, yes I noticed that previously and fixed it 👍 We should be quite close to succeeding now 🤞
Let's see what the pending e2e run reports.

vincepri
vincepri approved these changes Feb 9, 2024
View reviewed changes
Copy link
Member

@vincepri vincepri left a comment

Choose a reason for hiding this comment

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

/lgtm

Great work!

/assign @Ankitasw @richardcase
for approval

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Feb 9, 2024
@damdo damdo requested a review from Ankitasw February 13, 2024 09:51
@Ankitasw
Copy link
Member

/approve

Thankyou @damdo for your work 👏

@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: Ankitasw

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@k8s-ci-robot k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Feb 13, 2024
@k8s-ci-robot k8s-ci-robot merged commit 50a7f5b into kubernetes-sigs:main Feb 13, 2024
18 checks passed
@damdo damdo mentioned this pull request Feb 13, 2024
Closed
@AndiDog AndiDog mentioned this pull request Oct 10, 2024
Closed
5 tasks
@AndiDog AndiDog mentioned this pull request Oct 22, 2024
Open
5 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@JoelSpeed JoelSpeed JoelSpeed left review comments

@nrb nrb nrb left review comments

@vincepri vincepri vincepri approved these changes

@AverageMarcus AverageMarcus Awaiting requested review from AverageMarcus

@richardcase richardcase Awaiting requested review from richardcase

@Ankitasw Ankitasw Awaiting requested review from Ankitasw

Assignees

@richardcase richardcase

@vincepri vincepri

@Ankitasw Ankitasw

Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. kind/feature Categorizes issue or PR as related to a new feature. lgtm "Looks good to me", indicates that a PR is ready to be merged. needs-priority release-note Denotes a PR that will be considered when it comes time to generate release notes. size/XL Denotes a PR that changes 500-999 lines, ignoring generated files.
Projects
None yet
Milestone
v2.4.0
Development

Successfully merging this pull request may close these issues.
8 participants
@damdo @dlipovetsky @richardcase @vincepri @nrb @Ankitasw @k8s-ci-robot @JoelSpeed