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

Support encrypted and signed user data #5599

Open
wants to merge 13 commits into
base: main
Choose a base branch
from

Conversation

TheRealFalcon
Copy link
Member

@TheRealFalcon TheRealFalcon commented Aug 8, 2024

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>)

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

Choose a reason for hiding this comment

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

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

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

Choose a reason for hiding this comment

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

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 Outdated Show resolved Hide resolved
@@ -0,0 +1,201 @@
.. _pgp:
Copy link
Member Author

Choose a reason for hiding this comment

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

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.

Copy link
Member

@holmanb holmanb left a comment

Choose a reason for hiding this comment

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

First pass review


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
Copy link
Member

Choose a reason for hiding this comment

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

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:

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
Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Member Author

Choose a reason for hiding this comment

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

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 Show resolved Hide resolved
@github-actions github-actions bot added the documentation This Pull Request changes documentation label Aug 15, 2024
@TheRealFalcon TheRealFalcon force-pushed the encrypted branch 2 times, most recently from 884b803 to 344c3bc Compare August 15, 2024 04:28
Copy link

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.)

@github-actions github-actions bot added the stale-pr Pull request is stale; will be auto-closed soon label Aug 30, 2024
@github-actions github-actions bot closed this Sep 6, 2024
@TheRealFalcon TheRealFalcon reopened this Sep 6, 2024
@TheRealFalcon TheRealFalcon removed the stale-pr Pull request is stale; will be auto-closed soon label Sep 6, 2024
@TheRealFalcon
Copy link
Member Author

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

@holmanb holmanb self-assigned this Sep 16, 2024
cloudinit/gpg.py Outdated
data=data,
update_env=self.env,
)
if require_signature and "gpg: Good signature" not in result.stderr:
Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Member Author

@TheRealFalcon TheRealFalcon Sep 18, 2024

Choose a reason for hiding this comment

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

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
Copy link
Member Author

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.

Copy link
Member

@holmanb holmanb left a comment

Choose a reason for hiding this comment

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

@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 Outdated Show resolved Hide resolved

# 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
Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Member Author

@TheRealFalcon TheRealFalcon Oct 9, 2024

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

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 Outdated Show resolved Hide resolved
cloudinit/user_data.py Outdated Show resolved Hide resolved
cloudinit/user_data.py Outdated Show resolved Hide resolved
cloudinit/util.py Show resolved Hide resolved
Copy link
Member

@holmanb holmanb left a comment

Choose a reason for hiding this comment

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

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

* 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
Copy link
Member

Choose a reason for hiding this comment

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

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
Copy link
Member Author

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

.. code-block:: bash

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

Copy link
Member

Choose a reason for hiding this comment

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

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

$ 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`
Copy link
Member

Choose a reason for hiding this comment

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

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
Copy link
Member Author

#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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
documentation This Pull Request changes documentation
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants