Review feedback.

Lots of tweaks suggested by Menno, should clean things up.
MalloZup · Jan 30, 2017 · 6a5b1f4 · 6a5b1f4
1 parent e5bb962
commit 6a5b1f4
Show file tree

Hide file tree

Showing 6 changed files with 94 additions and 113 deletions.
diff --git a/cmd/juju/commands/ssh_common.go b/cmd/juju/commands/ssh_common.go
@@ -398,12 +398,7 @@ func (c *SSHCommon) reachableAddressGetter(entity string) (string, error) {
 	if !c.noHostKeyChecks {
 		publicKeys, err = c.apiClient.PublicKeys(entity)
 		if err != nil {
-			// We ignore NotFound errors, as we may not have finished registering
-			// keys. If they are truly important, they'll be trapped in the
-			// generateKnownHosts section.
-			if !errors.IsNotFound(err) && !params.IsCodeNotFound(err) {
-				return "", errors.Trace(err)
-			}
+			return "", errors.Annotatef(err, "retrieving SSH host keys for %q", entity)
 		}
 	}
 

diff --git a/cmd/juju/commands/ssh_unix_test.go b/cmd/juju/commands/ssh_unix_test.go
@@ -244,41 +244,34 @@ func (s *SSHSuite) TestSSHWillWorkInUpgrade(c *gc.C) {
 
 /// XXX(jam): 2017-01-25 do we need these functions anymore? We don't really
 //support ssh'ing to V1 anymore
-/// func (s *SSHSuite) TestSSHCommandHostAddressRetryAPIv1(c *gc.C) {
-/// 	s.setHostDialerFunc(func(address string) error {
-/// 		return network.NoAddressError("public")
-/// 	})
-/// 	s.setForceAPIv1(true)
-///
-/// 	s.testSSHCommandHostAddressRetry(c, false)
-/// }
-///
-/// func (s *SSHSuite) TestSSHCommandHostAddressRetryAPIv2(c *gc.C) {
-/// 	s.setHostDialerFunc(func(address string) error {
-/// 		return network.NoAddressError("available")
-/// 	})
-/// 	s.setForceAPIv1(false)
-///
-/// 	s.testSSHCommandHostAddressRetry(c, false)
-/// }
-///
-/// func (s *SSHSuite) TestSSHCommandHostAddressRetryProxyAPIv1(c *gc.C) {
-/// 	s.setHostDialerFunc(func(address string) error {
-/// 		return network.NoAddressError("private")
-/// 	})
-/// 	s.setForceAPIv1(true)
-///
-/// 	s.testSSHCommandHostAddressRetry(c, true)
-/// }
-///
-/// func (s *SSHSuite) TestSSHCommandHostAddressRetryProxyAPIv2(c *gc.C) {
-/// 	s.setHostDialerFunc(func(address string) error {
-/// 		return network.NoAddressError("available")
-/// 	})
-/// 	s.setForceAPIv1(false)
-///
-/// 	s.testSSHCommandHostAddressRetry(c, true)
-/// }
+func (s *SSHSuite) TestSSHCommandHostAddressRetryAPIv1(c *gc.C) {
+	// Start with nothing valid to connect to.
+	s.setHostChecker(validAddresses())
+	s.setForceAPIv1(true)
+
+	s.testSSHCommandHostAddressRetry(c, false)
+}
+
+func (s *SSHSuite) TestSSHCommandHostAddressRetryAPIv2(c *gc.C) {
+	s.setHostChecker(validAddresses())
+	s.setForceAPIv1(false)
+
+	s.testSSHCommandHostAddressRetry(c, false)
+}
+
+func (s *SSHSuite) TestSSHCommandHostAddressRetryProxyAPIv1(c *gc.C) {
+	s.setHostChecker(validAddresses())
+	s.setForceAPIv1(true)
+
+	s.testSSHCommandHostAddressRetry(c, true)
+}
+
+func (s *SSHSuite) TestSSHCommandHostAddressRetryProxyAPIv2(c *gc.C) {
+	s.setHostChecker(validAddresses())
+	s.setForceAPIv1(false)
+
+	s.testSSHCommandHostAddressRetry(c, true)
+}
 
 func (s *SSHSuite) testSSHCommandHostAddressRetry(c *gc.C, proxy bool) {
 	m := s.Factory.MakeMachine(c, nil)

diff --git a/network/ssh/package_test.go b/network/ssh/package_test.go
@@ -1,4 +1,4 @@
-// Copyright 2016 Canonical Ltd.
+// Copyright 2017 Canonical Ltd.
 // Licensed under the AGPLv3, see LICENCE file for details.
 
 package ssh_test

diff --git a/network/ssh/reachable.go b/network/ssh/reachable.go
@@ -57,22 +57,29 @@ func NewReachableChecker(dialer Dialer, timeout time.Duration) *reachableChecker
 // it uses the golang/x/crypto/ssh/HostKeyCallback to find the host keys on a
 // given connection.
 type hostKeyChecker struct {
+
 	// AcceptedKeys is a set of the Marshalled PublicKey content.
 	AcceptedKeys set.Strings
+
 	// Stop will be polled for whether we should stop trying to do any work
 	Stop <-chan struct{}
+
 	// HostPort is the identifier that corresponds to this connection
 	HostPort network.HostPort
+
 	// Accepted will be passed HostPort if it validated the connection
 	Accepted chan network.HostPort
+
 	// Dialer is a Dialer that allows us to initiate the underlying TCP connection
 	Dialer Dialer
+
 	// Finished will be set an event when we've finished our check (success or failure)
 	Finished chan struct{}
 }
 
 var hostKeyNotInList = errors.New("host key not in expected set")
 var hostKeyAccepted = errors.New("host key was accepted, retry")
+var hostKeyAcceptedButStopped = errors.New("host key was accepted, but search was stopped")
 
 func (h *hostKeyChecker) hostKeyCallback(hostname string, remote net.Addr, key ssh.PublicKey) error {
 	// Note: we don't do any advanced checking of the PublicKey, like whether
@@ -92,11 +99,12 @@ func (h *hostKeyChecker) hostKeyCallback(hostname string, remote net.Addr, key s
 		// first, still exit.
 		select {
 		case h.Accepted <- h.HostPort:
+			return hostKeyAccepted
 		case <-h.Stop:
+			return hostKeyAcceptedButStopped
 		}
-		return hostKeyAccepted
 	}
-	logger.Debugf("host key for %s not in our accepted set: use --debug --log-level=TRACE to see actual key", debugName)
+	logger.Debugf("host key for %s not in our accepted set: log at TRACE to see raw keys", debugName)
 	return hostKeyNotInList
 }
 
@@ -132,7 +140,9 @@ func (h *hostKeyChecker) Check() {
 	// TODO(jam): 2017-01-24 One limitation of our algorithm, is that we don't
 	// try to limit the negotiation of the keys to our set of possible keys.
 	// For example, say we only know about the RSA key for the remote host, but
-	// it has been updated to use a ECDSA key as well. We
+	// it has been updated to use a ECDSA key as well. Gocrypto/ssh might
+	// negotiate to use the "more secure" ECDSA key and we will see that
+	// as an invalid key.
 	sshconfig := &ssh.ClientConfig{
 		HostKeyCallback: h.hostKeyCallback,
 	}
@@ -172,10 +182,16 @@ type reachableChecker struct {
 	timeout time.Duration
 }
 
+// FindHost takes a list of possible host+port combinations and possible public
+// keys that the SSH server could be using. We make an attempt to connect to
+// each of those addresses and do an SSH handshake negotiation. We then check
+// if the SSH server's negotiated public key is in our allowed set. The first
+// address to successfully negotiate will be returned. If none of them succeed,
+// and error will be returned.
 func (r *reachableChecker) FindHost(hostPorts []network.HostPort, publicKeys []string) (network.HostPort, error) {
 	uniqueHPs := network.UniqueHostPorts(hostPorts)
 	successful := make(chan network.HostPort, 1)
-	stop := make(chan struct{}, 0)
+	stop := make(chan struct{})
 	// We use a channel instead of a sync.WaitGroup so that we can return as
 	// soon as we get one connected. We'll signal the rest to stop via the
 	// 'stop' channel.

diff --git a/network/ssh/reachable_test.go b/network/ssh/reachable_test.go
@@ -137,49 +137,12 @@ type Cleaner interface {
 
 // testTCPServer only listens on the socket, but doesn't speak SSH
 func testTCPServer(c *gc.C, cleaner Cleaner) network.HostPort {
-	listener, err := net.Listen("tcp", "127.0.0.1:0")
-	c.Assert(err, jc.ErrorIsNil)
-
-	listenAddress := listener.Addr().String()
+	listenAddress, shutdown := sshtesting.CreateTCPServer(c, func(tcpConn net.Conn) {
+		// We accept a connection, but then immediately close.
+		tcpConn.Close()
+	})
 	hostPort, err := network.ParseHostPort(listenAddress)
 	c.Assert(err, jc.ErrorIsNil)
-	c.Logf("listening on %q", hostPort)
-
-	shutdown := make(chan struct{}, 0)
-
-	go func() {
-		for {
-			select {
-			case <-shutdown:
-				// no more listening
-				c.Logf("shutting down %s", listenAddress)
-				listener.Close()
-				return
-			default:
-			}
-			// Don't get hung on Accept, set a deadline
-			if tcpListener, ok := listener.(*net.TCPListener); ok {
-				tcpListener.SetDeadline(time.Now().Add(1 * time.Second))
-			}
-			tcpConn, err := listener.Accept()
-			if err != nil {
-				if netErr, ok := err.(net.Error); ok {
-					if netErr.Timeout() {
-						// Try again, so we reevaluate if we need to shut down
-						continue
-					}
-				}
-			}
-			if err != nil {
-				c.Logf("failed to accept connection on %s: %v", listenAddress, err)
-				continue
-			}
-			if tcpConn != nil {
-				c.Logf("accepted tcp connection on %q from %s", hostPort, tcpConn.RemoteAddr())
-				tcpConn.Close()
-			}
-		}
-	}()
 	cleaner.AddCleanup(func(*gc.C) { close(shutdown) })
 
 	return *hostPort

diff --git a/network/ssh/testing/sshserver.go b/network/ssh/testing/sshserver.go
@@ -41,23 +41,12 @@ func denyPublicKey(conn ssh.ConnMetadata, key ssh.PublicKey) (*ssh.Permissions,
 	return nil, errors.Errorf("public key denied")
 }
 
-// CreateSSHServer launches an SSH server that will use the described private
-// key to allow SSH connections. Note that it explicitly doesn't actually
-// support any Auth mechanisms, so nobody can complete connections, but it will
-// do Key exchange to set up the encrypted conversation.
-// We return the address where the SSH service is listening, and a channel
-// callers must close when they want the service to stop.
-func CreateSSHServer(c *gc.C, privateKeys ...string) (string, chan struct{}) {
-	serverConf := &ssh.ServerConfig{
-		// We have to set up at least one Auth method, or the SSH server
-		// doesn't even try to do key-exchange
-		PublicKeyCallback: denyPublicKey,
-	}
-	for _, privateStr := range privateKeys {
-		privateKey, err := ssh.ParsePrivateKey([]byte(privateStr))
-		c.Assert(err, jc.ErrorIsNil)
-		serverConf.AddHostKey(privateKey)
-	}
+// CreateTCPServer launches a TCP server that just Accepts connections and
+// triggers the callback function. The callback assumes responsibility for
+// closing the connection.
+// We return the address+port of the TCP server, and a channel that can be
+// closed when you want the TCP server to stop.
+func CreateTCPServer(c *gc.C, callback func(net.Conn)) (string, chan struct{}) {
 	// We explicitly listen on IPv4 loopback instead of 'localhost'
 	listener, err := net.Listen("tcp", "127.0.0.1:0")
 	c.Assert(err, jc.ErrorIsNil)
@@ -93,16 +82,41 @@ func CreateSSHServer(c *gc.C, privateKeys ...string) (string, chan struct{}) {
 				continue
 			}
 			remoteAddress := tcpConn.RemoteAddr().String()
-			c.Logf("accepted tcp connection for ssh on %s from %s", localAddress, remoteAddress)
-			sshConn, _, _, err := ssh.NewServerConn(tcpConn, serverConf)
-			if err != nil {
-				// TODO: some errors are expected, as we don't support Auth
-				c.Logf("error initiating ssh connection for %s: %v", remoteAddress, err)
-			} else {
-				// We don't expect to get here, but if we do, make sure we close the connection.
-				sshConn.Close()
-			}
+			c.Logf("accepted tcp connection on %s from %s", localAddress, remoteAddress)
+			callback(tcpConn)
 		}
 	}()
 	return localAddress, shutdown
 }
+
+// CreateSSHServer launches an SSH server that will use the described private
+// key to allow SSH connections. Note that it explicitly doesn't actually
+// support any Auth mechanisms, so nobody can complete connections, but it will
+// do Key exchange to set up the encrypted conversation.
+// We return the address where the SSH service is listening, and a channel
+// callers must close when they want the service to stop.
+func CreateSSHServer(c *gc.C, privateKeys ...string) (string, chan struct{}) {
+	serverConf := &ssh.ServerConfig{
+		// We have to set up at least one Auth method, or the SSH server
+		// doesn't even try to do key-exchange
+		PublicKeyCallback: denyPublicKey,
+	}
+	for _, privateStr := range privateKeys {
+		privateKey, err := ssh.ParsePrivateKey([]byte(privateStr))
+		c.Assert(err, jc.ErrorIsNil)
+		serverConf.AddHostKey(privateKey)
+	}
+	return CreateTCPServer(c, func(tcpConn net.Conn) {
+		remoteAddress := tcpConn.RemoteAddr().String()
+		c.Logf("initiating ssh handshake for %s", remoteAddress)
+		sshConn, _, _, err := ssh.NewServerConn(tcpConn, serverConf)
+		if err != nil {
+			// TODO: some errors are expected, as we don't support Auth, we
+			// should probably not log things that aren't genuine errors.
+			c.Logf("error initiating ssh connection for %s: %v", remoteAddress, err)
+		} else {
+			// We don't expect to get here, but if we do, make sure we close the connection.
+			sshConn.Close()
+		}
+	})
+}