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

Draft: encryption at rest #3193

Merged
merged 7 commits into from
May 31, 2018
Merged

Draft: encryption at rest #3193

merged 7 commits into from
May 31, 2018

Conversation

mberhault
Copy link
Contributor

Some rudimentary description of encryption at rest, including full
example to:

  • generate keys
  • enable encryption
  • change encryption

There is much left to do.

The section on the admin UI stores page requires
cockroachdb/cockroach#26040

@cockroach-teamcity
Copy link
Member

This change is Reviewable

@jseldess jseldess requested a review from rmloveland May 25, 2018 14:20
@mberhault
Copy link
Contributor Author

CC: @nstewart, @rmloveland

Here's the direct link to the generated page: http://cockroach-docs-review.s3-website-us-east-1.amazonaws.com/ec7ef6a82f633b8a2717bf825428b9adee3d8ba8/dev/encryption.html

Nate: this should be enough to get you started, although we still need cockroachdb/cockroach#26040 to get merged (I'm hoping today).

Rich: I'll address the TODOs, there's quite a bit of background and explanation needed.

@mberhault
Copy link
Contributor Author

The stores report PR is merged.

@mberhault
Copy link
Contributor Author

Improved background section. I'll keep adding more.

@mberhault
Copy link
Contributor Author

FYI: this lives under: 2.1 -> Guides -> Deploy -> Enterprise Features -> Encryption At Rest.
Boy, we've really gone hierarchy crazy.

Some rudimentary description of encryption at rest, including full
example to:
* generate keys
* enable encryption
* change encryption

There is much left to do.

The section on the admin UI stores page requires
cockroachdb/cockroach#26040
@mberhault mberhault force-pushed the marc/draft_encryption_at_rest branch 2 times, most recently from 9a3ab6b to 1f124cb Compare May 29, 2018 17:30
@mberhault mberhault force-pushed the marc/draft_encryption_at_rest branch from 1f124cb to d66cf0a Compare May 29, 2018 17:34
@@ -330,6 +330,12 @@
"urls": [
"/${VERSION}/roles.html"
]
},
{
"title": "Encryption At Rest",
Copy link
Contributor

Choose a reason for hiding this comment

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

It would be good to add "(Alpha)" after the page title (or the appropriate feature status if not alpha)

@@ -0,0 +1,127 @@
---
title: Encryption At Rest
Copy link
Contributor

Choose a reason for hiding this comment

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

Same as previous comment - page title should end with "(Alpha)" or similar.

| Level | Description |
|-|-|
| Store keys | Provided by the user in a file. They are used to encrypt the list of data keys (see below).<br><br>This is known as a **key encryption key**: it's only purpose is to encrypt other keys.<br><br>Store keys are never persisted by CockroachDB.<br><br>Since very little data is encrypted using this key, it can have a very long lifetime without risk of reuse. |
| Data keys | Automatically generated by CockroachDB. They are used to encrypt all files on disk.<br><br> This is known as a **data encryption key**.<br><br>Data keys are persisted in a key registry file, encrypted using the store key.<br><br>The key has a short lifetime to avoid key/IV reuse. |
Copy link
Contributor

Choose a reason for hiding this comment

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

Does IV mean this? https://en.wikipedia.org/wiki/Initialization_vector

If so I suggest that could be a link for non-wizards.

@mberhault
Copy link
Contributor Author

Thanks for the comments Rich, but I just want to make sure we're on the same page: this is not even close to finished, and it's definitely not meant to be the final docs, more of a brain-dump on my part and as such will probably require some serious rewriting by people better at explaining than I am :)

@nstewart
Copy link
Contributor

Yes, at this point we just need enough docs to unblock pm acceptance testing and eng qa

@jseldess
Copy link
Contributor

@nstewart, do you still want some initial docs for next Monday's alpha release as well? Or is that no longer a priority?

@nstewart
Copy link
Contributor

nstewart commented May 29, 2018

Sorry for the confusion -- just wanted to make sure that we weren't expecting Marc's output to be user-facing today. We want people to be able to use this by the time the alpha comes around for early feedback

@rmloveland
Copy link
Contributor

Thanks for the comments Rich, but I just want to make sure we're on the same page: this is not even close to finished, and it's definitely not meant to be the final docs, more of a brain-dump on my part and as such will probably require some serious rewriting by people better at explaining than I am :)

I understand. Sorry to comment too early, I wasn't sure how we usually work on these draft PRs. I will stand back and wait for the go-ahead to review later or take over the PR when ready (or however it's done). :-)

@mberhault
Copy link
Contributor Author

No worries. I'll let you know when I've done the majority of the page. Either today or tomorrow.

@mberhault
Copy link
Contributor Author

The draft is pretty much ready. It will most likely need some serious improvements, but this should be a good start to understand how encryption at rest works, including examples and recommendations.

@rmloveland
Copy link
Contributor

Review status: 0 of 2 files reviewed at latest revision, 3 unresolved discussions, all commit checks successful.


v2.1/encryption.md, line 20 at r4 (raw file):

Encryption is performed in the [storage layer](architecture/storage-layer.html) and configured per store.
All files used by the store, regarless of contents, are encrypted with the desired algorithm.

s/regarless/regardless/


v2.1/encryption.md, line 43 at r4 (raw file):

* prevent key reuse with the same encryption parameters (after many files)

Markdown isn't rendering properly for this list. Maybe try an empty line before the list?


v2.1/encryption.md, line 50 at r4 (raw file):

Data keys will automatically be rotated at startup if any of the following conditions are met:
* the active store key changed

Same comment as above re: Markdown list rendering


v2.1/encryption.md, line 72 at r4 (raw file):

## Recommendations

There are a number of considerations to keep in mind when running with encryption:

s/:/./ since no list follows


v2.1/encryption.md, line 75 at r4 (raw file):

Key management is the most dangerous aspect of encryption. The following rules should be kept in mind:
* make sure only the unix user running the `cockroach` process has access to the keys

Markdown list render issue (same as above)


v2.1/encryption.md, line 81 at r4 (raw file):

A few other recommendations apply for best security practices:
* do not switch from encrypted to plaintext, this leaks data keys. Once transitioned to plaintext, all data must be considered reachable.

Markdown list render issue (same as above)


v2.1/encryption.md, line 168 at r4 (raw file):

key=/path/to/my/old-aes-128.key

do you mean old-key=/path/to/my/old-aes-128.key?


v2.1/encryption.md, line 173 at r4 (raw file):

## See Also

TODO(mberhault): links to external resources, report page details, flag descriptions.

Could replace this with something simple for now e.g.

- [Enterprise Licensing](enterprise-licensing.html)

Comments from Reviewable

@rmloveland
Copy link
Contributor

@mberhault if you prefer, I can take over the PR and finish making edits

@mberhault
Copy link
Contributor Author

Yes please, I'm deep into debugging an issue with encryption, I won't have time to look at this again today or tomorrow.

@nstewart
Copy link
Contributor

v2.1/encryption.md, line 84 at r4 (raw file):

* do not copy the encrypted files as the data keys are not easily available.
* if encryption is desired, start a node with it enabled from first run without ever running in plaintext.

@mberhault -- we need to include a sentence or two for dealing with backups. What happens when I take a backup of a secure cluster? If I want to encrypt backups, what would be our recommendation? Just use encrypted s3? zip up the files and then encrypt that?

cc @rmloveland


Comments from Reviewable

@mberhault
Copy link
Contributor Author

Review status: 0 of 2 files reviewed at latest revision, 12 unresolved discussions, all commit checks successful.


v2.1/encryption.md, line 84 at r4 (raw file):

Previously, nstewart (Nate) wrote…

@mberhault -- we need to include a sentence or two for dealing with backups. What happens when I take a backup of a secure cluster? If I want to encrypt backups, what would be our recommendation? Just use encrypted s3? zip up the files and then encrypt that?

cc @rmloveland

Backup (as in our distributed backup) still works as expected and does not encrypt anything. Encrypting the actual backups will be a completely different feature that likely reuses nothing from this. Ditto for import/export/dump.

Copying raw files from disk when encryption is turned on is now impossible (it's mentioned in the doc).


Comments from Reviewable

@nstewart
Copy link
Contributor

v2.1/encryption.md, line 84 at r4 (raw file):

Previously, mberhault (marc) wrote…

Backup (as in our distributed backup) still works as expected and does not encrypt anything. Encrypting the actual backups will be a completely different feature that likely reuses nothing from this. Ditto for import/export/dump.

Copying raw files from disk when encryption is turned on is now impossible (it's mentioned in the doc).

Right -- I'm not talking about copying raw files, I'm talking about working with the files created by the distributed (unencrypted) backup. My question is, even if we don't have a feature for encrypting backups, what would be our recommendation for people that want to encrypt the backed up files?


Comments from Reviewable

@mberhault
Copy link
Contributor Author

Review status: 0 of 2 files reviewed at latest revision, 12 unresolved discussions, all commit checks successful.


v2.1/encryption.md, line 84 at r4 (raw file):

Previously, nstewart (Nate) wrote…

Right -- I'm not talking about copying raw files, I'm talking about working with the files created by the distributed (unencrypted) backup. My question is, even if we don't have a feature for encrypting backups, what would be our recommendation for people that want to encrypt the backed up files?

It depends entirely on whether they trust the storage used as the backup destination. If they don't trust AWS/GCE/etc..., they'll need to encrypt the backups themselves. I don't think we want to provide more guidance than that though.


Comments from Reviewable

@nstewart
Copy link
Contributor

v2.1/encryption.md, line 84 at r4 (raw file):

Previously, mberhault (marc) wrote…

It depends entirely on whether they trust the storage used as the backup destination. If they don't trust AWS/GCE/etc..., they'll need to encrypt the backups themselves. I don't think we want to provide more guidance than that though.

I think that's enough. Basically just saying: hey, backups/exports/etc aren't encrypted; you will have to encrypt those using your favorite encryption method.

cc @rmloveland


Comments from Reviewable

@rmloveland
Copy link
Contributor

Review status: 0 of 2 files reviewed at latest revision, 12 unresolved discussions, all commit checks successful.


v2.1/encryption.md, line 84 at r4 (raw file):

I think that's enough. Basically just saying: hey, backups/exports/etc aren't encrypted; you will have to encrypt those using your favorite encryption method.

Just added a note to that effect. Thanks Nate!


Comments from Reviewable

@rmloveland rmloveland requested a review from jseldess May 31, 2018 16:28
@rmloveland
Copy link
Contributor

:lgtm:


Review status: 0 of 2 files reviewed at latest revision, 12 unresolved discussions, all commit checks successful.


Comments from Reviewable

Copy link
Contributor

@jseldess jseldess left a comment

Choose a reason for hiding this comment

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

Definitely LGTM for alpha docs, with some nits. Thanks, @rmloveland and @mberhault!

| AES-192 | 192 bits (24 bytes) | 56 bytes |
| AES-256 | 256 bits (32 bytes) | 64 bytes |

Generating a key file can be done using the `cockroach` CLI::
Copy link
Contributor

Choose a reason for hiding this comment

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

Remove extra :

Copy link
Contributor

Choose a reason for hiding this comment

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

Fixed!

$ cockroach start --store=cockroach-data --enterprise-encryption=path=cockroach-data,key=/path/to/my/aes-128.key,old-key=plain
~~~

**WARNING**: once specified for a given store, the `--enterprise-encryption` flag must always be present.
Copy link
Contributor

Choose a reason for hiding this comment

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

Let's convert this into a danger callout:

{{site.data.alerts.callout_danger}}Once specified for a given store, the <code>--enterprise-encryption</code> flag must always be present.{{site.data.alerts.end}}

Copy link
Contributor

Choose a reason for hiding this comment

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

Fixed!


<div id="toc"></div>

## Background
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: We use Overview for sections like this in other docs.

Copy link
Contributor

Choose a reason for hiding this comment

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

Fixed!


CockroachDB does not currently force re-encryption of older files but instead relies on normal RocksDB churn to slowly rewrite all data with the desired encryption.

## Key rotation
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: Rotating keys would make this parallel in form to Changing encryption type

Copy link
Contributor

Choose a reason for hiding this comment

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

Fixed

@rmloveland rmloveland merged commit dec77b3 into master May 31, 2018
@rmloveland rmloveland deleted the marc/draft_encryption_at_rest branch May 31, 2018 21:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants