Skip to content

Legacy encryption key support for when KMS is on#1223

Merged
pkarman merged 1 commit intomasterfrom
pek-kms-legacy
Mar 15, 2017
Merged

Legacy encryption key support for when KMS is on#1223
pkarman merged 1 commit intomasterfrom
pek-kms-legacy

Conversation

@pkarman
Copy link
Copy Markdown
Contributor

@pkarman pkarman commented Mar 14, 2017

Why: Environments with existing encrypted accounts will
fail to decrypt properly when KMS is turned on. This change
prefixes the new KMS accounts with 'KMSx' flag so that
they are recognizable beyond respecting the feature flag.

@jrminton
Copy link
Copy Markdown

This seems like it adds complexity to the algorithms. Could we go straight KMS and get rid of this special use case? The only reason I ask is that we are early enough now that we could make a case with partners, etc. that we blow away the databases one last time before starting with KMS. I wanted to ask the question to make sure I was not missing anything.

@pkarman
Copy link
Copy Markdown
Contributor Author

pkarman commented Mar 14, 2017

Fair question @jrminton. We need local encryption for environments w/o AWS KMS (like a local laptop install). The tagging of encryption keys also helps us future-proof beyond KMS.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Should we remove this Other key since it's not currently used?

@JohnRahaghi
Copy link
Copy Markdown

For Enable KMS #189, all the accounts in all the envs will be toasted. All the UUIDs will change. Do we need to preserve them?

@pkarman pkarman changed the title [WIP] Legacy encryption key support for when KMS is on Legacy encryption key support for when KMS is on Mar 15, 2017
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

My first question when skimming this "XOR with what?". Should we specify it's 'KMSx' XOR '0000'

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Since there's only one value in KEY_TYPE for now, should we just make this KMS_KEY_TYPE = 'KMSx'? Or is this part of the plan to refactor EncryptedKeyMaker to allow locally-encrypted keys too?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

the latter. we wanted to provide structure for future refactor.

**Why**: Environments with existing encrypted accounts will
fail to decrypt properly when KMS is turned on. This change
prefixes the new KMS accounts with 'KMSx' flag so that
they are recognizable beyond respecting the feature flag.
@pkarman
Copy link
Copy Markdown
Contributor Author

pkarman commented Mar 15, 2017

all ready @monfresh and @zachmargolis

Copy link
Copy Markdown
Contributor

@monfresh monfresh left a comment

Choose a reason for hiding this comment

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

lgtm

@pkarman pkarman merged commit 772a397 into master Mar 15, 2017
@pkarman pkarman deleted the pek-kms-legacy branch March 15, 2017 18:14
pkarman added a commit that referenced this pull request Mar 20, 2017
**Why**: Environments with existing encrypted accounts will
fail to decrypt properly when KMS is turned on. This change
prefixes the new KMS accounts with 'KMSx' flag so that
they are recognizable beyond respecting the feature flag.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants