feature: change encryption keys by ryanuber · Pull Request #199 · hashicorp/serf

ryanuber · 2014-04-11T08:13:33Z

For a week or so I've been chipping away at this feature little by little. It uses the new memberlist keyring feature to allow Serf to handle encryption key changes in a running cluster. I feel like it's feature-complete, and ready to start gathering some suggestions on the code itself.

There is more conversation around the feature in general in #194.

The bottom line on what it does can be seen with a few command examples. All of the operations are idempotent, so you can just keep running them without negative consequences if they fail.

Install a new key

$ serf key -install E4SoXyZSJkcg37n42U2i2Q==
==> Installing key on all members...
==> Successfully installed key!

Change the key used to encrypt messages:

$ serf key -use E4SoXyZSJkcg37n42U2i2Q==
==> Changing primary key on all members...
==> Successfully changed primary key!

List keys in use on the cluster:

$ serf key -list
==> Asking all members for installed keys...
==> Keys gathered, listing cluster keys...

E4SoXyZSJkcg37n42U2i2Q==
9kHC3w5eswavy1iU56YG8g==

Remove a key:

$ serf key -remove 9kHC3w5eswavy1iU56YG8g==
==> Removing key on all members...
==> Successfully removed key!

Error conditions during key operations:

$ serf key -remove E4SoXyZSJkcg37n42U2i2Q==
==> Removing key on all members...
failed:  db9    Removing the primary key is not allowed
failed:  web15  Removing the primary key is not allowed
failed:  db8    Removing the primary key is not allowed
failed:  web21  Removing the primary key is not allowed
failed:  lb31   Removing the primary key is not allowed
failed:  lb3    Removing the primary key is not allowed

Error removing key: 6/102 nodes reported failure

I know this pull request is pretty massive, so if splitting it into smaller chunks is preferred, I can try doing that. Just let me know!

armon · 2014-04-11T18:32:42Z

@ryanuber Wow! This is a huge PR! Awesome work! I will try to comb through this over the weekend.

With the -list mode, does it indicate if some nodes are missing certain keys? e.g. How does the operator verify all nodes have a given key before trying to -use that key?

ryanuber · 2014-04-11T18:48:32Z

@armon the -list command just gives you a list of all unique keys in use on the cluster. It doesn't tell you if they are in use on some but not others. You could get that confidence using the -install command though, since it will only return 0 if every node replies with success.

My thinking was that if the commands are idempotent, you would just do a -install until you succeeded. Similarly, you would want to do a -use until it succeeded before doing a -remove of the old key. So essentially, -install could also be looked at as a "verify" of sorts, "verify this key exists on all nodes".

armon · 2014-04-11T18:50:20Z

@ryanuber Got it. Would it be possible to add a very simple indicator to list, just something thats like [58/60] or something that indicates how many nodes have that key? This way an operator can quickly see if they need to run -install again.

ryanuber · 2014-04-11T18:52:25Z

@armon definitely! I'll take a stab at that over the weekend.

armon · 2014-04-14T05:49:55Z

serf/internal_query.go

I'd make this INFO level

armon · 2014-04-14T06:03:37Z

@ryanuber I've made a few notes, but overall this looks super solid! Very excited to have this in for the next release. Thanks for the hard work!

ryanuber · 2014-04-15T05:28:13Z

@armon Thanks for the review! I made some adjustments based on your recommendations and added a few response comments. Let me know what you think and we can adjust further from here.

armon · 2014-04-16T05:10:00Z

website/source/docs/agent/options.html.markdown

We should probably add a section below that documents the format of this file. Just so people know how to setup the keys, that the first key is the primary, etc.

armon · 2014-04-16T05:26:08Z

@ryanuber This is looking really good. I think it's very close. I think we are also missing some tests for the RPC layer, but I can always add those after a merge if you want.

…e mechanics are different from a tags file

…ved keys

…-file

… polluting the Serf API

…uest type

…ommands

…ions

ryanuber · 2014-04-18T08:09:04Z

@armon I took a swing at the RPC tests and added some command tests. If there is anything you think we are missing just let me know!

armon · 2014-04-18T18:16:51Z

@ryanuber Looks awesome! Thanks so much for all the hard work!

feature: change encryption keys

ryanuber mentioned this pull request Apr 11, 2014

feature: rotate encryption keys #194

Closed

armon reviewed Apr 14, 2014
View reviewed changes

serf/internal_query.go Outdated

Copy link

Member

armon Apr 14, 2014

Choose a reason for hiding this comment

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

I'd make this INFO level

armon reviewed Apr 16, 2014
View reviewed changes

ryanuber added 8 commits April 15, 2014 23:11

Initial work using memberlist keyring

33e1aa7

Added basic ability to use a -keyring-file

5668a34

agent: added keyring writer, might need to move it into Serf since th…

9f5db93

…e mechanics are different from a tags file

agent: fixed call to base64 encode (doesn't return errors)

b47e22b

serf: implement WriteKeyringFile in Serf so that we can persist recei…

1cddd50

…ved keys

command: added use-key skeleton

0078582

command: use-key is now a functional command, and works with -keyring…

a870ba4

…-file

serf: only write keyring file if it is configured

abf711b

ryanuber added 25 commits April 15, 2014 23:11

serf: added comments on undocumented functions

5ff7886

Move key manipulation functionality into a KeyManager object to avoid…

175533d

… polluting the Serf API

Relay number of nodes with a given encryption key installed

afdf549

Squashed identical response types into one type, removed unneeded req…

6007c80

…uest type

Commented types in keymanager

bdca549

Added r/w mutex to keyManager

af82446

serf: store a pointer to a keyring manager so the RWLock works

311dc97

command: rename 'serf key' to 'serf keys' for uniformity with other c…

d892201

…ommands

serf: writeKeyringFile is private now, and takes no arguments

8f59d4e

serf: implement key broadcast and response handling in reusable funct…

ce47deb

…ions

serf: mark query logs as INFO rather than DEBUG

fc45d23

website: added RPC documentation for key operations

aa54599

command: use newer columnize api

d5c15b6

command: protect against passing ambiguous arguments to keys command

17e8fb1

serf: return early when all nodes respond before query timeout

db060d5

serf: remove unused message types

d756172

Added keyring file format documentation

9b7528c

serf: use encoded struct as query payload for key operations

c1205a3

serf: handle responding to key-related queries using a common function

c741506

serf: use a goto inside of the channel loop to handle failure cases

ad6a008

Improve documentation and help output for keys command -list option

5a006da

serf: make RWMutex private in keymanager

0d3767b

Always return allocated response object to avoid nil pointer dereference

ab5d959

agent: first pass at rpc tests

85a7528

command: added tests for keys command

f575528

serf: added keyring file writer tests

94ec6dd

armon added a commit that referenced this pull request Apr 18, 2014

Merge pull request #199 from ryanuber/keyring

069de11

feature: change encryption keys

armon merged commit 069de11 into hashicorp:master Apr 18, 2014

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feature: change encryption keys#199

feature: change encryption keys#199
armon merged 65 commits intohashicorp:masterfrom
ryanuber:keyring

ryanuber commented Apr 11, 2014

Uh oh!

armon commented Apr 11, 2014

Uh oh!

ryanuber commented Apr 11, 2014

Uh oh!

armon commented Apr 11, 2014

Uh oh!

ryanuber commented Apr 11, 2014

Uh oh!

armon Apr 14, 2014

Uh oh!

armon commented Apr 14, 2014

Uh oh!

ryanuber commented Apr 15, 2014

Uh oh!

armon Apr 16, 2014

Uh oh!

armon commented Apr 16, 2014

Uh oh!

ryanuber commented Apr 18, 2014

Uh oh!

armon commented Apr 18, 2014

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Conversation

ryanuber commented Apr 11, 2014

Uh oh!

armon commented Apr 11, 2014

Uh oh!

ryanuber commented Apr 11, 2014

Uh oh!

armon commented Apr 11, 2014

Uh oh!

ryanuber commented Apr 11, 2014

Uh oh!

armon Apr 14, 2014

Choose a reason for hiding this comment

Uh oh!

armon commented Apr 14, 2014

Uh oh!

ryanuber commented Apr 15, 2014

Uh oh!

armon Apr 16, 2014

Choose a reason for hiding this comment

Uh oh!

armon commented Apr 16, 2014

Uh oh!

ryanuber commented Apr 18, 2014

Uh oh!

armon commented Apr 18, 2014

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants