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.

Sign up for GitHub

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

Jump to bottom

Serf encryption #1615

Closed
wants to merge 22 commits into from
Closed

Serf encryption #1615

wants to merge 22 commits into from

Conversation

Gerrrr
Copy link
Contributor

@Gerrrr Gerrrr commented Aug 18, 2016

Following the discussion at #469, this is the first PR which enables serf encryption.
Most of the code is taken from Consul codebase.

User-visible changes:

  • new server-specific CLI option for nomad agent - -encrypt
  • new server-specific configuration option encrypt
  • new CLI option nomad keygen which generates new encryption keys

Example configuration:

server.hcl

data_dir = "/var/lib/nomad"
server {
    encrypt = "pd3LCI6Um4o6KrxEva7ylw=="
    enabled = true
    bootstrap_expect = 3
    start_join = ["172.28.128.3", "172.28.128.4", "172.28.128.5"]
}

server01.hcl

bind_addr = "172.28.128.3"

server02.hcl

bind_addr = "172.28.128.4"

server03.hcl

bind_addr = "172.28.128.5"

@dadgar dadgar added this to the v0.5.0 milestone Aug 19, 2016
dadgar
dadgar reviewed Aug 19, 2016
View reviewed changes
command/agent/command.go
@@ -212,6 +213,13 @@ func (c *Command) readConfig() *Config {
}
}

if config.Server.Enabled && config.Server.EncryptKey != "" {
if _, err := config.EncryptBytes(); err != nil {
Copy link
Contributor

Choose a reason for hiding this comment

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

Should this go in setupKeyring?

@dadgar
Copy link
Contributor

dadgar commented Aug 19, 2016 •
edited
Loading

This branch does not build properly

@kevincox
Copy link

It built fine for me.

@kevincox
Copy link

So it seems to be working for me. I'm going to try running a job but status commands and the like are working.

Also IIUC this only encrypts serf traffic? Do you have plans for HTTP and RPC encryption in this PR or will that be coming at a different time?

@Gerrrr
Copy link
Contributor Author

Gerrrr commented Aug 22, 2016

@kevincox yes, this PR only adds Serf traffic encryption. I am also working on the PR for RPC traffic encryption, probably will publish it this week.

@Gerrrr Gerrrr changed the title Serf encryption [WIP] Serf encryption Aug 22, 2016
@dadgar
Copy link
Contributor

dadgar commented Aug 22, 2016

@kevincox The tests don't build, I should have clarified

@kevincox
Copy link

kevincox commented Aug 22, 2016 •
edited
Loading

@dadgar Ah, I see. I guess I just built the binary.

@Gerrrr Awesome. I would be ready to start running it "in production" once RPC is also done. Even if the HTTP isn't protected. I'll open an issue for the HTTP as there should probably be a discussion on the best way to do that. (#1639)

@kevincox kevincox mentioned this pull request Aug 22, 2016
Open
@Gerrrr Gerrrr changed the title [WIP] Serf encryption Serf encryption Aug 24, 2016
@Gerrrr
Copy link
Contributor Author

Gerrrr commented Aug 24, 2016

@dadgar Can you please take another look at the PR?

dadgar
dadgar reviewed Aug 25, 2016
View reviewed changes
command/keygen.go
)

// KeygenCommand is a Command implementation that generates an encryption
// key for use in `consul agent`.
Copy link
Contributor

Choose a reason for hiding this comment

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

nomad

@dadgar
Copy link
Contributor

dadgar commented Aug 25, 2016

Added some more comments. Before this can be merged we will have to also document the new config fields and flags and new commands in the website/docs

@Gerrrr Gerrrr changed the title Serf encryption [WIP] Serf encryption Aug 26, 2016
dadgar
dadgar reviewed Sep 2, 2016
View reviewed changes
command/agent/config.go
@@ -195,6 +196,9 @@ type ServerConfig struct {
// DataDir is the directory to store our state in
DataDir string `mapstructure:"data_dir"`

// Encryption key to use for the Serf communication
EncryptKey string `mapstructure:"encrypt" json:"-"`
Copy link
Contributor

Choose a reason for hiding this comment

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

json:"-"?

@dadgar
Copy link
Contributor

dadgar commented Sep 2, 2016

Hey Aleksandr,

Appreciate the work you have done so far. Within the next couple days we will have some bandwidth to take this PR over. I think it is pretty core feature and think it is best if we complete the rest of the work. Please just leave this PR open and we will branch from it.

Thanks,
Alex

@Gerrrr Gerrrr changed the title [WIP] Serf encryption Serf encryption Oct 4, 2016
@diptanu
Copy link
Contributor

diptanu commented Oct 5, 2016

Closing in favor of #1791

@Gerrrr Thanks for your hard work on this. Please review the new PR I have opened, and let us know what you think. We implemented a few things differently than how Consul has implemented this feature especially our API around key management is http based instead of Consul's RPC based interface.

@diptanu diptanu closed this Oct 5, 2016
@github-actions
Copy link

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.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Apr 16, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers
No reviews
Assignees

@diptanu diptanu

Labels
None yet
Projects
None yet
Milestone
v0.5.0
Development

Successfully merging this pull request may close these issues.
4 participants
@Gerrrr @dadgar @kevincox @diptanu