Interpolation functions rsaencrypt and rsadecrypt

[meyertime]

This is the second half of the puzzle which was previously #5675.

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

The first half is hashicorp/terraform-provider-aws#2219 which enables you to get (Windows) password data from an instance. This data is encrypted, and we don't want the decrypted password (or private key) to end up in the state file.

So to get around this, I added interpolation functions to allow decryption of this password data when setting the password in the connection section. Since that section is not stored in the state file, we're ok.

So, for example, once both of these PR's are merged, you can do this:

resource "aws_instance" "foo" {
  ...

  connection {
    ...

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

Or:

resource "null_resource" "foo" {
  ...

  connection {
    ...

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

Technically, we only need rsadecrypt to enable this feature, but I also added a matching rsaencrypt function while I was at it.

This PR is ready to merge, containing updated docs and working unit tests.

[smastrorocco]

+1

[apparentlymart]

Thanks for working on this, @deftflux!

I have some comments inline. Since this is crypto stuff I want to make sure I understand fully what's going on here, so I apologize that I'm being a bit "picky" here.

While I do like the symmetry of having both encrypt and decrypt functions, since this is a security-sensitive change I think I'd lean towards starting with just rsadecrypt -- which we than then review/test carefully while considering its intended use-case -- and hold off on rsaencrypt for now until there's a strong use-case for it. For crypto functionality in particular we need to be careful not to give users a false sense of security by explaining fully the intended use of each function, and I don't feel like I could tell a good story about rsaencrypt right now.

[apparentlymart]

It seems like you take a different approach to load the key in this function vs. the one below.

In this case we check to see if the key is encrypted and, I think, implicitly require it to be RSA by using ParsePKCS1PrivateKey. In the other function we don't check for an encrypted key and we use ParsePXIXPublicKey which I think accepts DSA and ECDSA keys too, which would then crash on the .(*rsa.PublicKey) type assertion underneath.

Does this work differently than I think here, or is there a reason why the two should be different?

[apparentlymart]

Assuming base64 for the ciphertext seems reasonable since it'll probably not be valid UTF-8, but in that case I'd expect rsaencrypt to be symmetrical and produce a base64 result as output, which it doesn't seem to. Are these asymmetrical for a reason?

[apparentlymart]

The docstring for EncryptPKCS1v15 contains this warning:

WARNING: use of this function to encrypt plaintexts other than session keys is dangerous. Use RSA OAEP in new protocols.

I don't have enough context to know whether that's important here, but since this is a crypto sort of thing I worry that we may end up passing whatever the limitation is on to Terraform users without disclosing it, which may cause problems for users down the line. I'd like to take some time to research better what the situation is before moving foward with this.

[apparentlymart]

We've had some bad experiences in the past with functions that except either a raw value or some encoded value, since it can cause surprising behavior if e.g. some input happens to look like base64 even though it was supposed to be raw ciphertext.

I think it would be better to just always require base64 input here, and then users can use base64encode if they have some unencoded ciphertext to deal with. Note though that Terraform only guarantees safe passage for strings that are valid UTF-8, so it's generally unsafe to send non-text content through Terraform.

[meyertime]

Thanks @apparentlymart for your feedback!

I'm totally ok with removing rsaencrypt for now. Like I already mentioned, it's not needed for the AWS Windows password decryption, which is what many Terraform Windows users have been wanting for a while. That will render some of your questions moot, but I'll answer all of them anyway just FYI.

As far as base64 encoding, it can't really be symmetrical, since a function can take different types of input, but it can only return one type of output. (In theory, the binary cipher should never be valid base64...) But I had to choose one to return from rsaencrypt, and I thought the raw cipher would be the more sensible default. However, I also didn't know that Terraform doesn't guarantee strings that aren't valid UTF-8; in that case, I agree that base64 all around would be better.

Regarding loading the keys, RSA is an asymmetric algorithm--the cipher is decrypted with a different key than it was encrypted with. Encryption is done with the public key, and decryption with the private key. (The only exception is when doing signature verification rather than encryption, but then typically the verbs "sign" and "verify" are used instead of "encrypt" and "decrypt".) The public key and private key are stored in slightly different formats; or, at least the format specifies whether it's a public or private key. So ParsePKCS1PrivateKey will not load a public key, and ParsePXIXPublicKey will not load a private key. That's the reason for the difference in loading. And you're right, if we were to keep rsaencrypt, we should put a guard around the type assertion to give a more friendly error message if the wrong type of public key is attempted.

Regarding the check to see if the key is encrypted, there is no need to check this for public keys, because public keys are supposed to be public, and so they are never encrypted. On the other hand, private keys are often encrypted with a password to avoid them being compromised in transit. By checking for this, we can give a more friendly error message in that case. To be honest, I actually copied this part of the code from Packer, which has long supported fetching and decrypting the Windows password data from AWS, and Packer also requires that the key not be encrypted. There is room to support decrypting the private key at a later time, though, either through another function or by adding an optional third parameter for the password to decrypt the private key. For the particular use case that motivates this pull request, though, it is not needed. AWS gives you private keys in plain unencrypted PEM format.

Regarding the warning about encrypting plaintexts, I am by no means an expert, but I found this page useful: https://security.stackexchange.com/questions/32050/what-specific-padding-weakness-does-oaep-address-in-rsa

It seems to me that PKCS #1 v1.5 padding has theoretical weaknesses that OAEP addresses, but it has been shown to be secure in practice. In any case, PKCS #1 v1.5 is the padding scheme that is used to encrypt the password data in AWS, so it is the padding scheme we need to be able to decrypt. However, we may want to call that out either in the documentation, in the name of the function (e.g. rsapkcs1v15decrypt -- rather ugly, but specific), or by adding a parameter for the padding scheme to use, like rsadecrypt("pkcs1v15", message, key).

I think that about covers it... So for now, I will remove rsaencrypt and change rsadecrypt to require base64-encoded cipher text.

[apparentlymart]

Hi @deftflux! Sorry I went quiet here; between the 0.11 release, the US thanksgiving holiday and then some illness I've got a bit behind here, but I'm looking at this now.

[apparentlymart]

Looking good! Thanks for making those updates and for the detailed response.

[red8888]

Can I pass the private key text in as a var instead of it being a file on disk? I'd like to not have the key in source control. I deploy terraform with jenkins which is capable of inject this as an environment var. Also, looks like this was merged, but when will it be added to the stable release?

[meyertime]

Yes @red8888 you can do as you please. I put file() in the example, since it's a common use case, but you can use any means of passing the key contents as a string into rsadecrypt().

It looks like rsadecrypt has been added to the release notes for 0.11.2, so in theory, it should be in the next release.

[meyertime]

Thanks @apparentlymart for merging. Now all that stands in the way of secure Windows provisioning in AWS with Terraform is merging hashicorp/terraform-provider-aws#2219 !

[ghost]

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 have found a problem that seems similar to this, please open a new issue and complete the issue template so we can capture all the details necessary to investigate further.