Fix for LP1921557 sni in Juju login.

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.
JoseFMP · Apr 8, 2021 · aa7520e · aa7520e
1 parent 9de8d59
commit aa7520e
Show file tree

Hide file tree

Showing 8 changed files with 68 additions and 20 deletions.
diff --git a/apiserver/facades/controller/caasoperatorprovisioner/provisioner_test.go b/apiserver/facades/controller/caasoperatorprovisioner/provisioner_test.go
@@ -257,7 +257,7 @@ func (s *CAASProvisionerSuite) TestIssueOperatorCertificate(c *gc.C) {
 		[]byte(certInfo.PrivateKey)...))
 	c.Assert(err, jc.ErrorIsNil)
 	c.Assert(len(signers), gc.Equals, 1)
-	c.Assert(len(certs), gc.Equals, 1)
+	c.Assert(len(certs), gc.Equals, 2)
 
 	roots := x509.NewCertPool()
 	ok := roots.AppendCertsFromPEM([]byte(certInfo.CACert))

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" {

diff --git a/worker/certupdater/certupdater_test.go b/worker/certupdater/certupdater_test.go
@@ -13,6 +13,7 @@ import (
 
 	jujucontroller "github.com/juju/juju/controller"
 	"github.com/juju/juju/core/network"
+	"github.com/juju/juju/pki"
 	pkitest "github.com/juju/juju/pki/test"
 	"github.com/juju/juju/state"
 	coretesting "github.com/juju/juju/testing"
@@ -105,7 +106,7 @@ func (s *CertUpdaterSuite) TestStartStop(c *gc.C) {
 	})
 	workertest.CleanKill(c, worker)
 
-	leaf, err := authority.LeafForGroup(certupdater.ControllerIPLeafGroup)
+	leaf, err := authority.LeafForGroup(pki.ControllerIPLeafGroup)
 	c.Assert(err, jc.ErrorIsNil)
 	c.Assert(leaf.Certificate().IPAddresses, coretesting.IPsEqual,
 		[]net.IP{net.ParseIP("192.168.1.1")})
@@ -126,7 +127,7 @@ func (s *CertUpdaterSuite) TestAddressChange(c *gc.C) {
 	// Certificate should be updated with the address value.
 
 	workertest.CleanKill(c, worker)
-	leaf, err := authority.LeafForGroup(certupdater.ControllerIPLeafGroup)
+	leaf, err := authority.LeafForGroup(pki.ControllerIPLeafGroup)
 	c.Assert(err, jc.ErrorIsNil)
 	c.Assert(leaf.Certificate().IPAddresses, coretesting.IPsEqual,
 		[]net.IP{net.ParseIP("0.1.2.3")})