-
Notifications
You must be signed in to change notification settings - Fork 9.6k
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
standalone client-side remote state encryption #28603
standalone client-side remote state encryption #28603
Conversation
…tate-encryption and deconflict
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks fantastic, great code coverage, great documentation. The security of the system is extremely well thought out and well-designed. I have only minor cosmetic suggestions.
} | ||
|
||
func (a *AES256StateWrapper) attemptDecryption(jsonCryptedData []byte, key []byte) ([]byte, error) { | ||
if !a.isSyntacticallyValidEncrypted(jsonCryptedData) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Cosmetic suggestion: This function is fairly long, which is OK, but I would suggest splitting it into more functions to improve readability. Perhaps "extractCiphertext", "extractIV", "decryptPayload", and "verifyIntegrity".
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for the suggestion. I have played around with this a bit, and in the end I only extracted the encoding and decoding to/from json. Further extractions didn't really improve readability much.
} | ||
|
||
// Encrypt data (which is a []byte containing a json structure) into a json structure | ||
// {"crypted":"<hex-encoded random iv + hex-encoded CFB encrypted data including hash>"} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Cosmetic suggestion:
I would also encode the encryption algorithm into the payload, denoting that the payload was encrypted with AES256, and include a version number, for revisions to the algorithm. Future payloads may be encrypted with other ciphers, but the same key, or perhaps the same key derivation scheme, but a different, future symmetric cipher.
I would use base64 encoding for the binary data, just to increase the density, but continue to allow it to be easily encoded as JSON.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am feeling slightly uneasy retrieving the decryption algorithm from the (potentially manipulated) remote state. It's probably safe, but I'd rather not. If key / algorithm rotation is desired or necessary, this can be done using the configuration pulled in from the environment variables.
if lastError != "" { | ||
t.Error("skipping test case, got error during instance creation: " + lastError) | ||
} else { | ||
if cut == nil { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Cosmetic suggestion, this is very deeply nested. I would consider extracting a function here for readability.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks! Done.
|
||
to enable client-side remote state encryption. To disable, either do not set the variable at all or set it to a blank value. | ||
|
||
Right now, there is only one value for `implementation`: `client-side/AES256-cfb/SHA256`. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Cosmetic, I would also capitalize "cfb", if you're capitalizing aes and sha.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done!
|
||
_Parameters:_ | ||
|
||
- `key`: the 32 byte AES256 key represented in hexadecimal, must be exactly 64 characters, `0-9a-f` only. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would also include, perhaps, a description of how to securely generate a key, since this doesn't use a key derivation algorithm. Perhaps a bash and powershell snippet, such as:
Bash
# Generate random bits with openssl
openssl rand -hex 32
# Generate random bits with /dev/urandom
tr -dc a-f0-9 </dev/urandom | head -c 64 ; echo ''
Powershell
$AESKey = New-Object Byte[] 32
[Security.Cryptography.RNGCryptoServiceProvider]::Create().GetBytes($AESKey)
($AESKey|ForEach-Object ToString x2) -join ''
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Too easy to google imho.
export TF_REMOTE_STATE_ENCRYPTION='{"implementation":"client-side/AES256-cfb/SHA256","parameters":{"key":"a0a1a2a3a4a5a6a7a8a9b0b1b2b3b4b5b6b7b8b9c0c1c2c3c4c5c6c7c8c9d0d1"}}' | ||
``` | ||
|
||
## State Encryption Lifecycle |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It would be nice, but I think, obviously, beyond the scope of this Pull Request, to create a CLI tool to help with these operations. Something like:
terraform state encrypt --implementation 'client-side/AES256-cfb/SHA256' --key 'a0a1a2a3a4a5a6a7a8a9b0b1b2b3b4b5b6b7b8b9c0c1c2c3c4c5c6c7c8c9d0d1'
> Encrypting state, implementation 'client-side/AES256-cfb/SHA256', key: a0a1a2a3a4a5a6a7a8a9b0b1b2b3b4b5b6b7b8b9c0c1c2c3c4c5c6c7c8c9d0d1
> Done
terraform state encrypt
> Encryption Implementations:
> 1. (Recommended) Client-side, AES256, CFB mode, SHA256 integrity check
> 2. Client-side, DES, ECB mode, no integrity check
> 3. Client-side, Pig Latin
>Which encryption implementation would you like to use (default: 1)?: 1
> Key Generation:
> 1. (Recommended) Generate a new completely random key
> 2. Generate a new key from a password PBKDF2
> 3. Use 4 (From random dice roll) https://xkcd.com/221/
> Which key generation algorithm would you like to use (default: 1)? 3
>
> Encrypting state, implementation 'client-side/AES256-cfb/SHA256', key: 0000000000000000000000000000000000000000000000000000000000000004
> Done
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If my PR gets accepted, I'm likely to build a simple encrypt / decrypt cli for downloaded state files/blobs. It is more or less required so state surgery is still possible.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@StephanHCB I honestly don't think yet another tool would be the best approach, but rather give terraform state pull
(https://www.terraform.io/docs/cli/commands/state/pull.html) the ability to dump the encrypted remote state locally and unencrypted - in short extend the existing terraform state handling commands to deal with encrypted remote state.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@StephanHCB I honestly don't think yet another tool would be the best approach, but rather give
terraform state pull
(https://www.terraform.io/docs/cli/commands/state/pull.html) the ability to dump the encrypted remote state locally and unencrypted - in short extend the existing terraform state handling commands to deal with encrypted remote state.
@frittentheke those commands already work
It's just that here are a number of edge case limitations, most of which also affect terraform state pull
with non-encrypted state, only with encryption you'll be completely locked out instead of being able to just externally download your state and manually merge changes with a previous version to recover.
Or consider bulk operations on a large number of state blobs, which naturally occur if you use terragrunt. It's really convenient to be able to just download all storage blobs from an Azure Storage container as a directory structure, then process them locally on the command line.
Y'all (Hashicorp) have been sitting on this since May of 2021 and it solves an open issue that is 7 years old! It solves the basic use case and the community has been asking for this. Please expedite this for inclusion in the next release. |
Huh, I signed that when I first made the original draft PR. In any case, I am absolutely thrilled to see progress on this PR. |
@StephanHCB fantastic work, many thanks! @StephanHCB @maludwig @crw do you have any idea when it might be merged into mainstream? Thanks! |
Hi @apparentlymart @jbardin any chance that you could comment on this MR please ? :-) |
Any view on when this might make live? |
I'm sadly just, like, a normal human and not super related to terraform. I simply reviewed the excellent code and gave it a thumbs up with some very minor suggestions. It's not up to me. |
I can understand Hashicorp being busy with other things and not wanting to maintain this if it doesn't align with their roadmap. It's a security sensitive thing so reviewing it, maintaining it, etc. is a lot of work. A way to make it easier to review and maintain is to off-load the crypto heavy-lifting to Google's Tink library. Which describes itself as a "open source library that provides cryptographic APIs that are secure, easy to use correctly, and hard(er) to misuse.". Because there is a lot that one can botch when using low-level crypto primitves to do encryption. Additionally perhaps it's an idea to add a flag to Terraform like |
Furthermore, if backends were a pluggable option (c.f. #5877) we could still maintain encryption as a non official tf plugin. |
Has there been any progress on this? Basically CMK encryption for terraform backend state is mandated for enterprise security compliance in many enterprises these days. We have one such case. If there is any workaround that would allow terraform to decrypt state files stored in a CMK-encrypted azure storage account that would be useful as well! Currently I'm getting this:
With this backend config:
|
Some light at the end of the tunnel: OpenTF promises to have this feature as a default. |
Are there any practices using this that would help a user or an organization avoid losing access to the backend state completely? Seems like if you lose the key to encrypt state, you also lose ability to manage anything using tf moving forward? And aren't there already practices to redact sensitive data from state? If anything, tightly restricting access to the state (i.e in S3) and logging accesses should suffice no? |
I think that's out of scope for Terraform and is something which requires similar precautions for all apps which use encrypted data. For example using WAL-G with PostgreSQL and uploading your backups to S3 has a similar problem. Having said that there are good solutions for mitigating this issue; For example you could print out the key on a piece of paper (as base64 and as a QR code), laminate the piece of paper and put it in a safe somewhere off site. USB sticks are typically not well suited for long term data storage since they store data by using a electric charge. This charge will fade over time and you might loose your data. An M-Disc is well suited for long term storage but is more hassle then using a piece of laminated paper. |
It's now been 2.5 years. I'm closing this stale PR. |
Is client side encryption explicitly a "won't fix" then? Or, is it just this specific implementation? It seems like it would be a generally useful addition to Terraform from my perspective. |
@dominic-p as the PR was closed by the original contributor (understandably), that is not a HashiCorp comment on whether or not the concept of client-side encryption is a "wont fix." These issues and PRs are frequently re-examined and discussed, though I have no new information to relay at this time. |
Thanks for the clarification. I completely missed who closed the PR. :) |
I'm going to lock this pull request because it has been closed for 30 days ⏳. This helps our maintainers find and focus on the active contributions. |
Proposal
See this comment on 9556
Also see the discussions on #9556 for the case for full client-side state encryption.
Summary:
This PR implements client side encryption of the entire state for all remote state storage backends except the enhanced backends.
What gets sent to the remote state storage just looks like this:
{"crypted":"e93e3e7ad343405525...dda4fc061"}
The idea is that even the company that operates the remote state storage cannot read it. Of course, one should still configure all other protection mechanisms on the remote storage, this is just one layer of security, but I think it's a very important one.
Features:
The change to existing code is minimal, see
states/remote/states.go
, I have basically inserted the encryption/decryption at the point at which Client.Get / Client.Put are invoked. As the result of the encryption is again json, this should hopefully work with all Clients with zero changes.Successfully tested with the azure backend.
Limitations
I have marked this feature experimental in the documentation, I do not have the resources to test it with all remote backends.
I assume it will not work with enhanced backends because these need access to the state.
.
I invite, and very much welcome, feedback from tf maintainers as to anything I might have overlooked or should do differently.