Fix for LP1921557 sni in Juju login.

tlm · tlm · commit 47fd6baefb56 · 2021-04-08T13:41:03.000+10:00
Juju login has been failing with recent changes to Juju's certificate
management. Specifically there are two problems that have come up.

The first is the Juju controller selecting the right certificate when ip
connections are being initiated that don't have an SNI name set. This
has been fixed by returning Juju's ip certificate with empty SNI names.

The second is Juju returning the CA cert in it's certificate chain.
Normally it's not considered valid to return a root CA in a chain but
for the way Juju works when logging in it needs to in order to trust the
CA and add it to the config file.
diff --git a/cmd/juju/user/login.go b/cmd/juju/user/login.go
@@ -8,6 +8,7 @@ import (
 	"crypto/x509"
 	"encoding/pem"
 	"fmt"
+	"net"
 	"net/http"
 	"os"
 	"strings"
@@ -362,10 +363,16 @@ func (c *loginCommand) publicControllerLogin(
 			dialOpts.BakeryClient.AddInteractor(i)
 		}
 
+		sniHost, _, err := net.SplitHostPort(host)
+		if err != nil {
+			return nil, errors.Annotatef(err, "getting sni host from host %q", host)
+		}
+
 		return apiOpen(&c.CommandBase, &api.Info{
-			Tag:      tag,
-			Password: d.Password,
-			Addrs:    []string{host},
+			Tag:         tag,
+			Password:    d.Password,
+			Addrs:       []string{host},
+			SNIHostName: sniHost,
 		}, dialOpts)
 	}
 	conn, accountDetails, err := c.login(ctx, currentAccountDetails, dial)
diff --git a/pki/authority.go b/pki/authority.go
@@ -18,7 +18,8 @@ import (
 )
 
 const (
-	DefaultLeafGroup = "controller"
+	DefaultLeafGroup      = "controller"
+	ControllerIPLeafGroup = "controllerip"
 )
 
 // Authority represents a secure means of issuing groups of common interest
@@ -80,6 +81,14 @@ func (a *DefaultAuthority) Chain() []*x509.Certificate {
 	return a.authority.Chain()
 }
 
+func (a *DefaultAuthority) ChainWithAuthority() []*x509.Certificate {
+	chain := a.authority.Chain()
+	if chain == nil {
+		chain = []*x509.Certificate{}
+	}
+	return append(chain, a.authority.Certificate())
+}
+
 // leafMaker is responsible for providing a method to make new leafs after
 // request signing.
 func (a *DefaultAuthority) leafMaker(groupKey string) LeafMaker {
@@ -104,11 +113,11 @@ func (a *DefaultAuthority) LeafRequestForGroup(group string) LeafRequest {
 	defer a.leafSignerMutex.Unlock()
 	if a.leafSigner != nil {
 		return NewDefaultLeafRequestWithSigner(subject, a.leafSigner,
-			NewDefaultRequestSigner(a.Certificate(), a.Chain(), a.Signer()),
+			NewDefaultRequestSigner(a.Certificate(), a.ChainWithAuthority(), a.Signer()),
 			a.leafMaker(groupKey))
 	}
 	return NewDefaultLeafRequest(subject,
-		NewDefaultRequestSigner(a.Certificate(), a.Chain(), a.Signer()),
+		NewDefaultRequestSigner(a.Certificate(), a.ChainWithAuthority(), a.Signer()),
 		a.leafMaker(groupKey))
 }
 
diff --git a/pki/authority_test.go b/pki/authority_test.go
@@ -76,6 +76,21 @@ func (a *AuthoritySuite) TestLeafRequest(c *gc.C) {
 	c.Assert(leaf.Certificate().IPAddresses, jc.DeepEquals, ipAddresses)
 }
 
+func (a *AuthoritySuite) TestLeafRequestChain(c *gc.C) {
+	authority, err := pki.NewDefaultAuthority(a.ca, a.signer)
+	c.Assert(err, jc.ErrorIsNil)
+	dnsNames := []string{"test.juju.is"}
+	ipAddresses := []net.IP{net.ParseIP("fe80:abcd::1")}
+	leaf, err := authority.LeafRequestForGroup("testgroup").
+		AddDNSNames(dnsNames...).
+		AddIPAddresses(ipAddresses...).
+		Commit()
+
+	chain := leaf.Chain()
+	c.Assert(len(chain), gc.Equals, 1)
+	c.Assert(chain[0], jc.DeepEquals, authority.Certificate())
+}
+
 func (a *AuthoritySuite) TestLeafFromPem(c *gc.C) {
 	authority, err := pki.NewDefaultAuthority(a.ca, a.signer)
 	c.Assert(err, jc.ErrorIsNil)
diff --git a/pki/tls/sni.go b/pki/tls/sni.go
@@ -21,14 +21,28 @@ type Logger interface {
 func AuthoritySNITLSGetter(authority pki.Authority, logger Logger) func(*tls.ClientHelloInfo) (*tls.Certificate, error) {
 	return func(hello *tls.ClientHelloInfo) (*tls.Certificate, error) {
 		logger.Debugf("received tls client hello for server name %s", hello.ServerName)
+
 		var cert *tls.Certificate
-		authority.LeafRange(func(leaf pki.Leaf) bool {
-			if err := hello.SupportsCertificate(leaf.TLSCertificate()); err == nil {
-				cert = leaf.TLSCertificate()
-				return false
+
+		// NOTE: This was added in response to bug lp:1921557. If we get an
+		// empty server name here we assume the the connection is being made
+		// with an ip address as the host.
+		if hello.ServerName == "" {
+			logger.Debugf("tls client hello server name is empty. Attempting to provide ip address certificate")
+			leaf, err := authority.LeafForGroup(pki.ControllerIPLeafGroup)
+			if err != nil && !errors.IsNotFound(err) {
+				return nil, errors.Annotate(err, "fetching ip address certificate")
 			}
-			return true
-		})
+			cert = leaf.TLSCertificate()
+		} else {
+			authority.LeafRange(func(leaf pki.Leaf) bool {
+				if err := hello.SupportsCertificate(leaf.TLSCertificate()); err == nil {
+					cert = leaf.TLSCertificate()
+					return false
+				}
+				return true
+			})
+		}
 
 		if cert == nil {
 			logger.Debugf("no matching certificate found for tls client hello %s, using default certificate", hello.ServerName)
diff --git a/pki/tls/sni_test.go b/pki/tls/sni_test.go
@@ -89,6 +89,12 @@ func (s *SNISuite) TestAuthorityTLSGetter(c *gc.C) {
 			Group:         pki.DefaultLeafGroup,
 			ServerName:    "doesnotexist.juju.example",
 		},
+		{
+			// Regression test for LP1921557
+			ExpectedGroup: pki.ControllerIPLeafGroup,
+			Group:         pki.ControllerIPLeafGroup,
+			ServerName:    "",
+		},
 	}
 
 	for _, test := range tests {
diff --git a/worker/certupdater/certupdater.go b/worker/certupdater/certupdater.go
@@ -17,10 +17,6 @@ import (
 	"github.com/juju/juju/watcher/legacy"
 )
 
-const (
-	ControllerIPLeafGroup = "controllerip"
-)
-
 var (
 	logger = loggo.GetLogger("juju.worker.certupdater")
 )
@@ -112,7 +108,7 @@ func (c *CertificateUpdater) updateCertificate(addresses network.SpaceAddresses)
 	logger.Debugf("new machine addresses: %#v", addresses)
 	c.addresses = addresses
 
-	request := c.authority.LeafRequestForGroup(ControllerIPLeafGroup)
+	request := c.authority.LeafRequestForGroup(pki.ControllerIPLeafGroup)
 
 	for _, addr := range addresses {
 		if addr.Value == "localhost" {
