Support encrypted and signed user data #5599

TheRealFalcon · 2024-08-08T20:33:35Z

Proposed Commit Message

feat: Support encrypted and signed user data

Cloud-init user data often contains user secrets including passwords
and private keys. This data has always been submitted in plain text.
To protect this data's confidentiality and guarantee its authenticity,
add the ability to have this data encrypted and signed.

A new user data format is added allowing for an ASCII armored PGP
MESSAGE. If detected, cloud-init will import into a temporary keyring
any keys provided in /etc/cloud/keys and use these keys to decrypt
and/or verify the provided data.

After decryption, the resulting message will be treated as user data
as before.

Fixes GH-4943

Additional Context

The squash: commits are just to make reviewing easier. I plan to squash them before merging.

Test Steps

Merge type

Squash merge using "Proposed Commit Message"
Rebase and merge unique commits. Requires commit messages per-commit each referencing the pull request number (#<PR_NUM>)

TheRealFalcon · 2024-08-08T20:34:08Z

tests/unittests/cloudinit/test_user_data.py

@@ -0,0 +1,129 @@
+"""These tests are for code in the cloudinit.user_data module.


Note that I moved non-PGP related tests here as they fit better here.

TheRealFalcon · 2024-08-08T20:36:09Z

cloudinit/user_data.py

+            # that we attempt to decode said payload so that the transformed
+            # data can be examined.
+            parent_ctype = None
+            if ctype in TRANSFORM_TYPES:


I made a necessary refactor to this section involving setting the mime type of transformed messages. Please double check my logic 🙂 .

I did ensure that gzipped user data still works properly with this code.

cloudinit/user_data.py

TheRealFalcon · 2024-08-08T20:41:35Z

doc/rtd/howto/pgp.rst

@@ -0,0 +1,201 @@
+.. _pgp:


If running this guide, don't be like me and forget to install this branch into the container and then be mad it doesn't work at the end.

holmanb

First pass review

holmanb · 2024-08-13T21:20:29Z

doc/rtd/howto/pgp.rst

+
+Save this file to your working directory as `cloud-config.yaml`.
+
+Encrypt the user data using the public key of the encrypting user and the


Encrypt the user data using the public key of the encrypting user and the private key of the signing user:

The signing key doesn't do encryption. How about:

Encrypt the user data using the public key of the encrypting user and sign it using the private key of the signing user:

holmanb · 2024-08-13T21:21:59Z

doc/rtd/howto/pgp.rst

+using the existing key ring in the snapshot, we do this for a few reasons:
+
+* Users may not want these keys in any key ring by default on a new instance
+* Keys may be exported from any system without needing to be concerned with


I don't understand what this is trying to say.

I can export keys from a system I'm not making an instance from without trying to move gpg keyrings around that may not even be compatible with the target system.

I can wordsmith it.

cloudinit/gpg.py

github-actions · 2024-08-30T00:24:15Z

Hello! Thank you for this proposed change to cloud-init. This pull request is now marked as stale as it has not seen any activity in 14 days. If no activity occurs within the next 7 days, this pull request will automatically close.

If you are waiting for code review and you are seeing this message, apologies! Please reply, tagging TheRealFalcon, and he will ensure that someone takes a look soon.

(If the pull request is closed and you would like to continue working on it, please do tag TheRealFalcon to reopen it.)

TheRealFalcon · 2024-09-12T19:37:02Z

@holmanb , I have rebased the branch (only minor format.rst conflicts). It should be ready for review again.

holmanb · 2024-09-18T15:58:02Z

cloudinit/gpg.py

+            data=data,
+            update_env=self.env,
+        )
+        if require_signature and "gpg: Good signature" not in result.stderr:


I don't love depending on stderr for this behavior. Are there alternatives that might be better?

Updated. I thought the same thing as I wrote it but also couldn't think of a better idea. Not sure why I didn't think of the verify command sooner.

TheRealFalcon · 2024-09-26T20:53:21Z

Rebased away the conflict.

@holmanb , can I get priority on this review? We're getting close to the end of the cycle and I'd like to get this one done so I can focus on my other items with less context switching.

holmanb

@TheRealFalcon Thanks for this large effort!

I still need to work through the tutorial and tests, but so far no major blockers. I left a couple of comments / and nitpicks inline.

cloudinit/sources/__init__.py

holmanb · 2024-10-07T23:32:45Z

cloudinit/user_data.py


-            # Attempt to figure out the payloads content-type
-            if not ctype_orig:
-                ctype_orig = UNDEF_TYPE
            # There are known cases where mime-type text/x-shellscript included
            # non shell-script content that was user-data instead.  It is safe
            # to check the true MIME type for x-shellscript type since all


It is safe
to check the true MIME type for x-shellscript type since all
shellscript payloads must have a #! header. The other MIME types
that cloud-init supports do not have the same guarantee.

I know that you didn't write this, but we should be careful with assumptions like this. I think that this comment is saying that a shebang is required for executable scripts on Linux which makes it safe to check the content type when it is type text/x-shellscript. If that's what this is saying, then this could be a bug, because the "shebang is required for shell scripts" assumption is untrue:

$ echo "echo no-shebang" > test.txt $ chmod +x test.txt $ ./test.txt no-shebang

This statement does seem dubious:

# There are known cases where mime-type text/x-shellscript included # non shell-script content that was user-data instead.

It might be worth digging a little more into this to see if we know how this could happen, and if this is, indeed, a bug (and if so, then we should document it).

I don't think that this is specifically a bug in the code that you've added here, but when I see questionable comments and assumption, I get a little bit more concerned about the code quality - therefore the modifications that we are making.

If that's what this is saying, then this could be a bug, because the "shebang is required for shell scripts" assumption is untrue

Is it? Trying to pass a mime message made from cloud-init devel make-mime -a test.sh:x-shellscript with script having no shebang leads to a cc_scripts_user failure due to a subp failure:

ProcessExecutionError("Exec format error. Missing #! in script?\nCommand: ['/var/lib/cloud/instance/scripts/test.sh']\nExit code: -\nReason: [Errno 8] Exec format error: b'/var/lib/cloud/instance/scripts/test.sh'\nStdout: -\nStderr: -")

I think http://www.faqs.org/faqs/unix-faq/faq/part3/section-16.html explains why.

If that's what this is saying, then this could be a bug, because the "shebang is required for shell scripts" assumption is untrue

Is it? Trying to pass a mime message made from cloud-init devel make-mime -a test.sh:x-shellscript with script having no shebang leads to a cc_scripts_user failure due to a subp failure:

ProcessExecutionError("Exec format error. Missing #! in script?\nCommand: ['/var/lib/cloud/instance/scripts/test.sh']\nExit code: -\nReason: [Errno 8] Exec format error: b'/var/lib/cloud/instance/scripts/test.sh'\nStdout: -\nStderr: -")

I think http://www.faqs.org/faqs/unix-faq/faq/part3/section-16.html explains why.

No you're right. That link describes csh, but dash's man page describes this behavior as well. This is a thing that we could support if we wanted to with a subshell, but I don't think we need to.

I still have questions about what "known cases where mime-type text/x-shellscript included shell-script content that was user-data instead". This sounds like a potential bug.

cloudinit/user_data.py

cloudinit/util.py

holmanb

Thanks for addressing the latest comments @TheRealFalcon.

I'm still[1] not comfortable with the complexity introduced by supporting individually signed MIME parts. Doing this turns cloud-init's mime parsing code (which is complex and not well documented) into a security boundary. I think it would be safer to disallow part-level signing/encryption, and require that the whole user-data message be signed.

Using the example included in this PR, consider a platform that passes along a user's signed and encrypted mime alongside their own malicious part-handler payload.

If this code worked correctly, the part-handler wouldn't be loaded, yet that isn't the case in the system I tested this on:

# ls -1 /var/tmp/ | grep txt
hacked.txt
hello-world.txt

[1] https://docs.google.com/document/d/1f8SUkgXgO0Fv62Nbgquz8gnGu_EtKhVPTJaaN_6rrXU/edit?disco=AAABSP2snRU

holmanb · 2024-10-14T22:13:10Z

doc/rtd/howto/pgp.rst

+* Users may not want these keys in any key ring by default on a new instance
+* Exporting keys is easier than copying key rings
+
+Note that on launch, cloud-init will import there keys into a temporary


nit: "will import these keys"

Rather than an top-level `allow_userdata` key, instead use a `user_data` dict. This is to better align with the top-level `vendor_data` keys.

Cloud-init user data often contains user secrets including passwords and private keys. This data has always been submitted in plain text. To protect this data's confidentiality and guarantee its authenticity, this commit add the ability to have this data encrypted and signed. A new user data format is added allowing for an ASCII armored PGP MESSAGE. If detected, cloud-init will import into a temporary keyring any keys provided in /etc/cloud/keys and use these keys to decrypt and/or verify the provided data. After decryption, the resulting message will be treated as user data as before.

TheRealFalcon · 2024-10-15T17:44:12Z

@holmanb , thanks for the comment. I have updated the PR accordingly.

holmanb · 2024-10-19T00:02:25Z

doc/rtd/howto/pgp.rst

+.. code-block:: bash
+
+    $ gpg --export-secret-keys encrypting_user > /etc/cloud/keys/encrypting_user.gpg
+


Think we should remove these keys from the root keychain after exporting them?

holmanb · 2024-10-19T00:03:02Z

doc/rtd/howto/pgp.rst

+    $ lxc launch pgp-demo-image pgp-demo-encrypted \
+      --config user.user-data="$(cat cloud-config.yaml.asc)"
+
+On the launched system, you should see the file `/var/tmp/hello-world.txt`


I just ran through this tutorial (making sure to remove the gpg keys from the root keychain), but it failed for me:

Traceback (most recent call last): File "/usr/lib/python3/dist-packages/cloudinit/gpg.py", line 106, in decrypt subp.subp( File "/usr/lib/python3/dist-packages/cloudinit/subp.py", line 291, in subp raise ProcessExecutionError( cloudinit.subp.ProcessExecutionError: Unexpected error while running command. Command: ['gpg', '--verify'] Exit code: 2 Reason: - Stdout: Stderr: gpg: verify signatures failed: Unexpected error The above exception was the direct cause of the following exception: Traceback (most recent call last): File "/usr/lib/python3/dist-packages/cloudinit/cmd/main.py", line 820, in status_wrapper ret = functor(name, args) ^^^^^^^^^^^^^^^^^^^ File "/usr/lib/python3/dist-packages/cloudinit/cmd/main.py", line 487, in main_init init.update() File "/usr/lib/python3/dist-packages/cloudinit/stages.py", line 570, in update self._store_processeddata(self.datasource.get_userdata(), "userdata") ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ File "/usr/lib/python3/dist-packages/cloudinit/sources/__init__.py", line 650, in get_userdata self.userdata = self.ud_proc.process( ^^^^^^^^^^^^^^^^^^^^^ File "/usr/lib/python3/dist-packages/cloudinit/user_data.py", line 98, in process convert_string( File "/usr/lib/python3/dist-packages/cloudinit/user_data.py", line 452, in convert_string bdata = handle_encrypted(bdata, is_part, require_signature) ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ File "/usr/lib/python3/dist-packages/cloudinit/user_data.py", line 413, in handle_encrypted return decrypt_payload(data.decode("utf-8"), require_signature).encode( ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ File "/usr/lib/python3/dist-packages/cloudinit/user_data.py", line 393, in decrypt_payload return gpg_context.decrypt( ^^^^^^^^^^^^^^^^^^^^ File "/usr/lib/python3/dist-packages/cloudinit/gpg.py", line 113, in decrypt raise GpgVerificationError( cloudinit.gpg.GpgVerificationError: Signature verification failed

TheRealFalcon · 2024-10-21T14:07:32Z

#5599 (comment)

My "fix" to this comment broke things. I think I had initially seen this which is why I relied on stderr in the first place but then forget in the time between adding it and seeing your comment. I found a better alternative to stderr and pushed it.

I updated an integration test accordingly and the tutorial should be working again.

github-actions · 2024-11-05T00:26:03Z

Hello! Thank you for this proposed change to cloud-init. This pull request is now marked as stale as it has not seen any activity in 14 days. If no activity occurs within the next 7 days, this pull request will automatically close.

If you are waiting for code review and you are seeing this message, apologies! Please reply, tagging TheRealFalcon, and he will ensure that someone takes a look soon.

(If the pull request is closed and you would like to continue working on it, please do tag TheRealFalcon to reopen it.)

TheRealFalcon commented Aug 8, 2024

View reviewed changes

cloudinit/user_data.py Outdated Show resolved Hide resolved

TheRealFalcon commented Aug 8, 2024

View reviewed changes

holmanb reviewed Aug 14, 2024

View reviewed changes

github-actions bot added the documentation This Pull Request changes documentation label Aug 15, 2024

TheRealFalcon force-pushed the encrypted branch 2 times, most recently from 884b803 to 344c3bc Compare August 15, 2024 04:28

github-actions bot added the stale-pr Pull request is stale; will be auto-closed soon label Aug 30, 2024

github-actions bot closed this Sep 6, 2024

TheRealFalcon reopened this Sep 6, 2024

TheRealFalcon removed the stale-pr Pull request is stale; will be auto-closed soon label Sep 6, 2024

TheRealFalcon force-pushed the encrypted branch from 344c3bc to 36719d8 Compare September 12, 2024 19:36

holmanb self-assigned this Sep 16, 2024

holmanb reviewed Sep 18, 2024

View reviewed changes

TheRealFalcon force-pushed the encrypted branch from f3be313 to a1186a8 Compare September 26, 2024 20:51

holmanb reviewed Oct 8, 2024

View reviewed changes

TheRealFalcon mentioned this pull request Oct 10, 2024

[enhancement]: support signed and encrypted userdata #4943

Open

holmanb requested changes Oct 14, 2024

View reviewed changes

TheRealFalcon added 8 commits October 15, 2024 08:35

chore: deprecate 'allow_userdata', add 'user_data'

92471e9

Rather than an top-level `allow_userdata` key, instead use a `user_data` dict. This is to better align with the top-level `vendor_data` keys.

squash: Add require_gpg flag

038bb20

squash: tests and docs

3bb1a0a

spelling lint

8f64f9d

comments

168e346

unit test failure and lint

4e2726a

stderr bad

e92beee

TheRealFalcon added 2 commits October 15, 2024 08:35

comments

126126d

remove mime processing, update tests and doc comment

7d40954

TheRealFalcon force-pushed the encrypted branch from 6ca0959 to 7d40954 Compare October 15, 2024 13:35

handle userdata list

ee2a368

holmanb reviewed Oct 19, 2024

View reviewed changes

TheRealFalcon added 2 commits October 21, 2024 08:41

unwrap before verify

5c8743a

update docs

2d60e45

TheRealFalcon requested a review from holmanb October 21, 2024 14:07

github-actions bot added the stale-pr Pull request is stale; will be auto-closed soon label Nov 5, 2024

TheRealFalcon removed the stale-pr Pull request is stale; will be auto-closed soon label Nov 6, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Support encrypted and signed user data #5599

Support encrypted and signed user data #5599

		@@ -0,0 +1,129 @@
		"""These tests are for code in the cloudinit.user_data module.


		Save this file to your working directory as `cloud-config.yaml`.

		Encrypt the user data using the public key of the encrypting user and the

		.. code-block:: bash

		$ gpg --export-secret-keys encrypting_user > /etc/cloud/keys/encrypting_user.gpg

Support encrypted and signed user data #5599

Are you sure you want to change the base?

Support encrypted and signed user data #5599

Conversation

Proposed Commit Message

Additional Context

Test Steps

Merge type

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment