Make verify_outgoing compatible with agents not configured for TLS #3001

kyhavlov · 2017-05-03T22:05:48Z

Fixes #1705 by allowing a graceful transition from a non-TLS configured cluster to a TLS one.

slackpad · 2017-05-03T22:41:28Z

consul/client.go

 		eventCh:    make(chan serf.Event, serfEventBacklog),
 		logger:     logger,
 		shutdownCh: make(chan struct{}),
 	}

+	c.connPool = NewPool(config.LogOutput, clientRPCConnMaxIdle, clientMaxStreams, tlsWrap, c.LANMembers)


I'd make this above and set it in the structure as connPool: connPool.

We need to pass in the LANMembers function from the client as a callback, though

Check out the comments below - I think we can move the "should use TLS" logic onto https://github.com/hashicorp/consul/blob/master/consul/agent/server.go and then just pass the result of that into dial and friends.

slackpad · 2017-05-03T22:45:05Z

consul/client.go

@@ -150,6 +151,10 @@ func (c *Client) setupSerf(conf *serf.Config, ch chan serf.Event, path string) (
 	conf.Tags["vsn_min"] = fmt.Sprintf("%d", ProtocolVersionMin)
 	conf.Tags["vsn_max"] = fmt.Sprintf("%d", ProtocolVersionMax)
 	conf.Tags["build"] = c.config.Build
+	conf.Tags["rpc_port"] = fmt.Sprintf("%d", c.config.RPCAddr.Port)


We don't ever inbound connect to a client agent, right? We probably don't need this here.

I think that's right, I thought the keyring or event stuff might use it, but those both use serf to gossip out, so we can remove this

Yeah - they don't have an inbound listener at this time.

slackpad · 2017-05-03T23:02:14Z

consul/pool.go

 	}

 	return conn, hc, nil
 }

+// memberExpectsTLS returns true if the member with the specified address is
+// verifying incoming TLS connections
+func memberExpectsTLS(addr string, members []serf.Member) (bool, error) {


This looks pretty expensive since it's scanning all the members. In other methods where we use the connPool, we actually have an agent.Server, which is essentially the Serf info - see https://github.com/hashicorp/consul/blob/master/consul/rpc.go#L259-L266. We could probably put the TLS logic in there, and pass that through.

Like as a useTLS Boolean maybe.

slackpad · 2017-05-03T23:03:09Z

consul/server.go

@@ -379,6 +380,7 @@ func (s *Server) setupSerf(conf *serf.Config, ch chan serf.Event, path string, w
 	conf.Tags["raft_vsn"] = fmt.Sprintf("%d", s.config.RaftConfig.ProtocolVersion)
 	conf.Tags["build"] = s.config.Build
 	conf.Tags["port"] = fmt.Sprintf("%d", addr.Port)
+	conf.Tags["rpc_port"] = fmt.Sprintf("%d", s.config.RPCAddr.Port)


I think this is the same as the existing port tag.

The existing tag has the LAN gossip port, not the RPC port

I don't think so - it gets populated from the RPC listener above (sanity check on https://demo.consul.io/v1/agent/self?pretty shows 8300).

slackpad · 2017-05-03T23:04:17Z

consul/server.go

@@ -388,6 +390,9 @@ func (s *Server) setupSerf(conf *serf.Config, ch chan serf.Event, path string, w
 	if s.config.NonVoter {
 		conf.Tags["nonvoter"] = "1"
 	}
+	if s.config.VerifyIncoming {
+		conf.Tags["tls_verify_incoming"] = "1"


Don't we need to key this off of certs being configured and have a sense more like use_tls? During the transition we will have to have verify turned off, but the server set up to handle incoming TLS connections.

You're right, this can be more permissive since we only really need to avoid trying to do outgoing TLS to an agent that doesn't have it configured at all

slackpad · 2017-05-03T23:05:38Z

Put some comments on the review - I think we need to work through the overall approach a little bit more and then do another round.

magiconair · 2017-05-04T00:25:02Z

consul/client.go

 		eventCh:    make(chan serf.Event, serfEventBacklog),
 		logger:     logger,
 		shutdownCh: make(chan struct{}),
 	}

+	c.connPool = NewPool(config.LogOutput, clientRPCConnMaxIdle, clientMaxStreams, tlsWrap, c.LANMembers)


magiconair · 2017-05-04T00:26:10Z

consul/pool.go

@@ -151,11 +157,13 @@ type ConnPool struct {
 // Set maxTime to 0 to disable reaping. maxStreams is used to control
 // the number of idle streams allowed.
 // If TLS settings are provided outgoing connections use TLS.
-func NewPool(logOutput io.Writer, maxTime time.Duration, maxStreams int, tlsWrap tlsutil.DCWrapper) *ConnPool {
+func NewPool(logOutput io.Writer, maxTime time.Duration, maxStreams int, tlsWrap tlsutil.DCWrapper,


Single line pls.

magiconair · 2017-05-04T00:28:03Z

consul/pool.go

-		// Switch the connection into TLS mode
-		if _, err := conn.Write([]byte{byte(rpcTLS)}); err != nil {
-			conn.Close()
+		doWrap, err := memberExpectsTLS(addr.String(), p.memberFunc())


This does a lot of magic. Please add a comment explaining what happens here and why.

magiconair · 2017-05-04T00:29:41Z

consul/raft_rpc.go

-		// Switch the connection into TLS mode
-		if _, err := conn.Write([]byte{byte(rpcTLS)}); err != nil {
-			conn.Close()
+		doWrap, err := memberExpectsTLS(string(address), l.memberFunc())


This looks identical to the previous code. helper fn?

magiconair · 2017-05-04T00:30:15Z

consul/server.go

@@ -270,6 +269,8 @@ func NewServer(config *Config) (*Server, error) {
 		shutdownCh:            make(chan struct{}),
 	}

+	s.connPool = NewPool(config.LogOutput, serverRPCCache, serverMaxStreams, tlsWrap, s.LANMembers)


I'm with James on that one. Create first, then inject.

slackpad · 2017-05-05T02:39:04Z

One other thing - will this take away TLS until everybody is upgraded (to have the proper tag)? I wonder if we do need that config after all (you'd still want the tag because it makes the upgrade seamless), but it seems like we need to turn this on to not break existing behavior. That, or we could add the tag now and enable the actual switching part in pool.go in a later version of Consul.

kyhavlov · 2017-05-05T03:01:27Z

We could set use_tls to 0 when verify_incoming is disabled and treat a missing tag as 1, which would be the existing behavior.

slackpad · 2017-05-05T03:06:44Z

We could set use_tls to 0 when verify_incoming is disabled and treat a missing tag as 1, which would be the existing behavior.

Or would missing tag == 1 be equivalent to now, even? If you have TLS configured it will try to use TLS. The mixed mode business only works if your whole cluster is upgraded.

slackpad · 2017-05-05T17:12:33Z

Would be good to add some new tests as well.

slackpad

Putting the logic in server.go should make this a lot more efficient and flows the information down in a good way. Put some open questions on here, and we also need to update the docs.

slackpad · 2017-05-05T20:25:46Z

consul/agent/server.go

@@ -40,6 +40,7 @@ type Server struct {
 	NonVoter    bool
 	Addr        net.Addr
 	Status      serf.MemberStatus
+	UseTLS      bool


This is worth some documentation about how it should be used.

slackpad · 2017-05-05T20:28:07Z

consul/server.go

+		s.localLock.RUnlock()
+
+		if !ok {
+			return false


This might be safer as return true, as if we don't know anything about the tag and defer to whether TLS is configured our not the old way (based on tlsWrap). Does that seem right?

Agree that defaulting to true (for TLS on) seems better, I thought I had it that way but must have changed it.

slackpad · 2017-05-05T20:29:13Z

consul/agent/server.go

@@ -72,6 +73,9 @@ func IsConsulServer(m serf.Member) (bool, *Server) {
 	datacenter := m.Tags["dc"]
 	_, bootstrap := m.Tags["bootstrap"]

+	tlsTag, ok := m.Tags["use_tls"]
+	useTLS := !ok || tlsTag == "1"


This is worth adding a few lines in the unit test.

slackpad · 2017-05-05T20:31:07Z

consul/server_test.go

+	defer os.RemoveAll(dir2)
+	defer s2.Shutdown()
+
+	// Try to join


You can convert this to use the new test helpers and retry stuff - that landed this morning.

slackpad · 2017-05-05T20:33:40Z

consul/server_test.go

+		c.CAFile = "../test/client_certs/rootca.crt"
+		c.CertFile = "../test/client_certs/server.crt"
+		c.KeyFile = "../test/client_certs/server.key"
+		c.VerifyOutgoing = true


Hmm - shouldn't this fail if you have verify_outgoing set and the other end doesn't support TLS? It seems like during migration you'd need to turn this off until everything was upgraded.

verify_outgoing needs to work for servers that don't have TLS on, otherwise we would need 2 intermediary steps instead of one.

I was thinking you'd leave both verify settings off, but configure the CA everywhere, then in a final step turn verify on. It seems like if you have verify_outgoing on, you really don't want to connect to anyone insecurely.

If you turn them both on at once with no interop, servers with verify_incoming will reject connections from the servers that don't have verify_outgoing set.

slackpad · 2017-05-09T22:29:20Z

command/agent/agent.go

+	if a.config.CAPath != "" || a.config.CAFile != "" {
+		base.VerifyOutgoing = true
+	}
+	base.ForceVerifyOutgoing = a.config.VerifyOutgoing


Instead of remapping VerifyOutgoing to ForceVerifyOutgoing how about keeping the sense of VerifyOutgoing the same and adding something like UseTLS?

slackpad · 2017-05-09T22:29:32Z

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

+
+## Configuring TLS on an existing cluster
+
+As of version 0.8.2, Consul supports migrating to TLS-encrypted traffic on a running cluster


slackpad · 2017-05-09T22:30:34Z

consul/server.go

@@ -393,6 +393,9 @@ func (s *Server) setupSerf(conf *serf.Config, ch chan serf.Event, path string, w
 	if s.config.NonVoter {
 		conf.Tags["nonvoter"] = "1"
 	}
+	if s.config.CAFile != "" || s.config.CAPath != "" {


This could key off of the s.config.UseTLS as well (see comment above).

slackpad

Noted some small stuff, but otherwise this is looking like it's almost there!

slackpad · 2017-05-10T14:53:39Z

consul/server_test.go

+	retry.Run(t, func(r *retry.R) { r.Check(wantPeers(s1, 2)) })
+
+	// Have s2 make an RPC call to s1
+	s2.localLock.RLock()


Need to unlock this.

slackpad · 2017-05-10T14:54:10Z

consul/server_test.go

+
+	testrpc.WaitForLeader(t, s1.RPC, "dc1")
+
+	// Add a second server with TLS and VerifyOutgoing set


Stale comment.

slackpad · 2017-05-10T14:54:59Z

consul/server_test.go

+		c.KeyFile = "../test/client_certs/server.key"
+		c.VerifyIncoming = true
+		c.VerifyOutgoing = true
+		c.UseTLS = true


This should be set automatically, right?

slackpad · 2017-05-10T14:55:15Z

consul/server_test.go

+
+	testrpc.WaitForLeader(t, s1.RPC, "dc1")
+
+	// Add a second server with TLS and VerifyOutgoing set


Stale comment.

slackpad · 2017-05-10T14:55:34Z

consul/server_test.go

+		c.CAFile = "../test/client_certs/rootca.crt"
+		c.CertFile = "../test/client_certs/server.crt"
+		c.KeyFile = "../test/client_certs/server.key"
+		c.UseTLS = true


Shouldn't need this, actually, since setting the CA will set this.

slackpad · 2017-05-10T14:57:09Z

tlsutil/config.go

@@ -49,6 +49,10 @@ type Config struct {
 	// existing clients.
 	VerifyServerHostname bool

+	// UseTLS is used to enable outgoing TLS verification of all outgoing connections to


This comment is a little misleading with the word "verification". I'd just say "UseTLS enables outgoing TLS connections to Consul servers."

slackpad · 2017-05-10T14:58:01Z

consul/server_test.go

+		c.CAFile = "../test/client_certs/rootca.crt"
+		c.CertFile = "../test/client_certs/server.crt"
+		c.KeyFile = "../test/client_certs/server.key"
+		c.UseTLS = true


Don't think we need to set this manually.

slackpad · 2017-05-10T14:59:15Z

consul/server.go

+		s.localLock.RUnlock()
+
+		if !ok {
+			return true


This should flip back to false with this new implementation. The verify_outgoing forces it, otherwise if we don't know anything about the other server then we probably don't want to use TLS.

slackpad

LGTM (I tweaked one comment)

Make verify_outgoing compatible with agents not configured for TLS

d1438b4

kyhavlov requested review from magiconair and slackpad May 3, 2017 22:05

slackpad reviewed May 3, 2017

View reviewed changes

magiconair suggested changes May 4, 2017

View reviewed changes

kyhavlov added 2 commits May 4, 2017 11:53

Rework outgoing TLS logic to use agent.Server from upstream

a7ec927

Merge branch 'master' into tls-outgoing-interop

ddfffa1

Add tests for verify_outgoing interop

fbaa8c7

slackpad reviewed May 5, 2017

View reviewed changes

kyhavlov added 3 commits May 5, 2017 13:58

Add UseTLS to agent.Server test

f901618

Merge branch 'master' into tls-outgoing-interop

8588611

Make verify_outgoing the default when TLS is enabled

0bd5a98

slackpad reviewed May 9, 2017

View reviewed changes

kyhavlov force-pushed the tls-outgoing-interop branch from dd7433f to ac0f187 Compare May 10, 2017 00:21

Change naming of force TLS variables

250008b

kyhavlov force-pushed the tls-outgoing-interop branch from ac0f187 to 250008b Compare May 10, 2017 00:40

slackpad reviewed May 10, 2017

View reviewed changes

kyhavlov and others added 3 commits May 10, 2017 10:49

Minor test cleanup

4510509

Merge branch 'master' into tls-outgoing-interop

75b3106

Tweaks a comment.

027025c

slackpad approved these changes May 10, 2017

View reviewed changes

kyhavlov merged commit 5bab68b into master May 10, 2017

kyhavlov deleted the tls-outgoing-interop branch May 10, 2017 21:25


		## Configuring TLS on an existing cluster

		As of version 0.8.2, Consul supports migrating to TLS-encrypted traffic on a running cluster


		testrpc.WaitForLeader(t, s1.RPC, "dc1")

		// Add a second server with TLS and VerifyOutgoing set

Make verify_outgoing compatible with agents not configured for TLS #3001

Make verify_outgoing compatible with agents not configured for TLS #3001

Conversation

kyhavlov commented May 3, 2017

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

slackpad commented May 3, 2017

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

slackpad commented May 5, 2017 • edited Loading

kyhavlov commented May 5, 2017

slackpad commented May 5, 2017

slackpad commented May 5, 2017

slackpad 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

slackpad 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

slackpad left a comment

Choose a reason for hiding this comment

slackpad commented May 5, 2017 •

edited

Loading