Enable serf encryption #1791

diptanu · 2016-10-04T03:30:09Z

@Gerrrr Please help us test this feature if you can!

dadgar

LGTM. Left small comments + tests failures should be fixed

dadgar · 2016-10-05T17:42:04Z

command/agent/config-test-fixtures/basic.hcl

@@ -68,6 +68,7 @@ server {
 	retry_max = 3
 	retry_interval = "15s"
 	rejoin_after_leave = true
+    encrypt = "abc"


Indentation

dadgar · 2016-10-05T17:43:40Z

nomad/structs/structs.go

+// KeyringResponse is a unified key response and can be used for install,
+// remove, use, as well as listing key queries.
+type KeyringResponse struct {
+	Messages map[string]string


Comment on these?

dadgar · 2016-10-05T17:44:12Z

command/keyring.go

+	helpText := `
+Usage: nomad keyring [options]
+
+  Manages encryption keys used for gossip messages between nomad servers. Gossip


Nomad should be capital

schmichael · 2016-10-05T17:41:08Z

api/agent.go

+	}
+	var resp KeyringResponse
+	_, err := a.client.write("/v1/agent/keyring/install", &args, &resp, nil)
+	return &resp, err


Above you do the extra if err != nil { return nil, err } check, so maybe keep all of these methods consistent in what they return?

schmichael · 2016-10-05T17:42:55Z

api/agent.go

+}
+
+// KeyringRequest is request objects for serf key operations.
+type KeyringRequest struct {


Unused? Looks like you use structs.KeyringRequest instead.

Using them now.

schmichael · 2016-10-05T17:49:43Z

command/agent/config-test-fixtures/basic.hcl

@@ -68,6 +68,7 @@ server {
 	retry_max = 3
 	retry_interval = "15s"
 	rejoin_after_leave = true
+    encrypt = "abc"


schmichael · 2016-10-05T17:56:21Z

command/agent/keyring.go

+	}
+	defer fh.Close()
+
+	if _, err := fh.Write(keyringBytes); err != nil {


You could just use ioutil.WriteFile instead of OpenFile+Write, but you'd still need the Remove on error.

schmichael · 2016-10-05T18:01:06Z

api/agent.go

@@ -16,6 +18,19 @@ type Agent struct {
 	region     string
 }

+// KeyringResponse is a unified key response and can be used for install,
+// remove, use, as well as listing key queries.
+type KeyringResponse struct {


Maybe just use the struct package's KeyringResponse instead?

@schmichael The API package mimics the structs package largely but gives a separation between internal and external structs

Well that explains it! Thanks. (Wish I could mark comments as addressed in this new PR mode without full on deleting them...)

schmichael · 2016-10-05T18:03:04Z

website/source/docs/agent/config.html.md

@@ -391,6 +391,16 @@ configured on client nodes.
    join any nodes when it starts up. Addresses can be given as an IP, a domain
    name, or an IP:Port pair. If the port isn't specified the default Serf port,
    4648, is used.  DNS names may also be used.
+  * <a id="encrypt">`encrypt`</a> Specifies the secret key to use for encryption
+    of Nomad server's gossip network traffic. This key must be 16-bytes that are


or 24 or 32? Or do we not even want to bother mentioning the aes 192 & 256 support?

schmichael · 2016-10-05T18:04:03Z

website/source/docs/commands/keyring.html.md.erb

+# Command: `keyring`
+
+The `keyring` command is used to examine and modify the encryption keys used in
+Nomad server. It is capable of


reformat lines

Gerrrr · 2016-10-06T09:38:48Z

command/agent/agent_endpoint_test.go

+			t.Fatalf("bad: %v", kresp)
+		}
+	})
+}


Does it make sense to also add a test for use operation?

Gerrrr · 2016-10-06T11:51:35Z

@diptanu Looks great!

Does it make sense to add general options to the keyring command? I was not able to use it when the server is not listening on local interface:

$ bin/nomad server-members -address=http://172.28.128.3:4646
Name                    Address       Port  Status  Leader  Protocol  Build     Datacenter  Region
nomad-server01.region1  172.28.128.3  4648  alive   false   2         0.5.0dev  dc1         region1
nomad-server02.region1  172.28.128.4  4648  alive   true    2         0.5.0dev  dc1         region1
nomad-server03.region1  172.28.128.5  4648  alive   false   2         0.5.0dev  dc1         region1

$ bin/nomad keyring -list
==> Gathering installed encryption keys...
error: Get http://127.0.0.1:4646/v1/agent/keyring/list: dial tcp 127.0.0.1:4646: getsockopt: connection refused

$ bin/nomad keyring -list -address=http://172.28.128.3:4646
flag provided but not defined: -address

diptanu · 2016-10-08T02:00:50Z

@Gerrrr Definitely, good catch! I can do that, only have internet access via cybercafes at the moment as I am on vacation, but consider it done by the time we make the final 0.5 release.

github-actions · 2023-04-16T02:13:01Z

I'm going to lock this pull request because it has been closed for 120 days ⏳. This helps our maintainers find and focus on the active contributions.
If you have found a problem that seems related to this change, please open a new issue and complete the issue template so we can capture all the details necessary to investigate further.

diptanu added 3 commits October 3, 2016 20:28

Added the keygen command

6705ab8

Added support for gossip encryption

1bc0c02

Changed the URL for keyring management

2c115a2

diptanu force-pushed the f-serf-encrypt branch from 203b1bb to 2c115a2 Compare October 4, 2016 03:44

diptanu added 3 commits October 3, 2016 21:01

Fixed the cli

7a5362d

Added some tests

1bc1b9b

Added tests for keyring operations

b269804

diptanu force-pushed the f-serf-encrypt branch from a3b34ea to b269804 Compare October 4, 2016 16:16

Added a test for removal of keys

e3dea06

diptanu mentioned this pull request Oct 5, 2016

Serf encryption #1615

Closed

Added some docs

8514abe

diptanu force-pushed the f-serf-encrypt branch from 5e854c7 to 8514abe Compare October 5, 2016 06:34

dadgar approved these changes Oct 5, 2016

View reviewed changes

schmichael reviewed Oct 5, 2016

View reviewed changes

Fixed some docs

35a02c0

Gerrrr reviewed Oct 6, 2016

View reviewed changes

Added general options

aa7c8c2

diptanu merged commit f0806dc into master Oct 17, 2016

diptanu deleted the f-serf-encrypt branch October 17, 2016 17:48

github-actions bot locked as resolved and limited conversation to collaborators Apr 16, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Enable serf encryption #1791

Enable serf encryption #1791

diptanu commented Oct 4, 2016 •

edited

Loading

dadgar left a comment

dadgar Oct 5, 2016

dadgar Oct 5, 2016

dadgar Oct 5, 2016

schmichael Oct 5, 2016

schmichael Oct 5, 2016

diptanu Oct 17, 2016

schmichael Oct 5, 2016

schmichael Oct 5, 2016

schmichael Oct 5, 2016

dadgar Oct 6, 2016

schmichael Oct 6, 2016

schmichael Oct 5, 2016

schmichael Oct 5, 2016

Gerrrr Oct 6, 2016

Gerrrr commented Oct 6, 2016

diptanu commented Oct 8, 2016

github-actions bot commented Apr 16, 2023

Enable serf encryption #1791

Enable serf encryption #1791

Conversation

diptanu commented Oct 4, 2016 • edited Loading

dadgar left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Gerrrr commented Oct 6, 2016

diptanu commented Oct 8, 2016

github-actions bot commented Apr 16, 2023

diptanu commented Oct 4, 2016 •

edited

Loading