Merge pull request from GHSA-4c49-9fpc-hc3v

Enable mTLS for server certificates while in PKI mode (4.0/stable)

lxd-agent/server.go

@@ -111,7 +111,7 @@ func authenticate(r *http.Request, cert *x509.Certificate) bool {
 	clientCerts := map[string]x509.Certificate{"0": *cert}
 
 	for _, cert := range r.TLS.PeerCertificates {
-		trusted, _ := util.CheckTrustState(*cert, clientCerts, nil, false)
+		trusted, _ := util.CheckMutualTLS(*cert, clientCerts)
 		if trusted {
 			return true
 		}

lxd/certificates.go

@@ -621,6 +621,14 @@ func certificatesPost(d *Daemon, r *http.Request) response.Response {
 		}
 
 		cert = r.TLS.PeerCertificates[len(r.TLS.PeerCertificates)-1]
+		networkCert := d.endpoints.NetworkCert()
+		if networkCert.CA() != nil {
+			// If we are in CA mode, we only allow adding certificates that are signed by the CA.
+			trusted, _, _ := util.CheckCASignature(*cert, networkCert)
+			if !trusted {
+				return response.Forbidden(fmt.Errorf("The certificate is not trusted by the CA or has been revoked"))
+			}
+		}
 	} else {
 		return response.BadRequest(fmt.Errorf("Can't use TLS data on non-TLS link"))
 	}

lxd/cluster/tls.go

@@ -70,13 +70,13 @@ func tlsCheckCert(r *http.Request, networkCert *shared.CertInfo, serverCert *sha
 		// member before the database is available. It also allows us to switch the server certificate to
 		// the network certificate during cluster upgrade to per-server certificates, and it be trusted.
 		trustedServerCert, _ := x509.ParseCertificate(serverCert.KeyPair().Certificate[0])
-		trusted, _ := util.CheckTrustState(*i, map[string]x509.Certificate{serverCert.Fingerprint(): *trustedServerCert}, networkCert, false)
+		trusted, _ := util.CheckMutualTLS(*i, map[string]x509.Certificate{serverCert.Fingerprint(): *trustedServerCert})
 		if trusted {
 			return true
 		}
 
 		// Check the trusted server certficates list provided.
-		trusted, _ = util.CheckTrustState(*i, trustedCerts[db.CertificateTypeServer], networkCert, false)
+		trusted, _ = util.CheckMutualTLS(*i, trustedCerts[db.CertificateTypeServer])
 		if trusted {
 			return true
 		}

lxd/daemon.go

@@ -298,7 +298,7 @@ func (d *Daemon) Authenticate(w http.ResponseWriter, r *http.Request) (bool, str
 	// Allow internal cluster traffic by checking against the trusted certfificates.
 	if r.TLS != nil {
 		for _, i := range r.TLS.PeerCertificates {
-			trusted, fingerprint := util.CheckTrustState(*i, trustedCerts[db.CertificateTypeServer], d.endpoints.NetworkCert(), false)
+			trusted, fingerprint := util.CheckMutualTLS(*i, trustedCerts[db.CertificateTypeServer])
 			if trusted {
 				return true, fingerprint, "cluster", nil
 			}
@@ -370,8 +370,19 @@ func (d *Daemon) Authenticate(w http.ResponseWriter, r *http.Request) (bool, str
 		return false, "", "", err
 	}
 
+	networkCert := d.endpoints.NetworkCert()
+	checkCertificateSignature := networkCert.CA() != nil
 	for _, i := range r.TLS.PeerCertificates {
-		trusted, username := util.CheckTrustState(*i, trustedCerts[db.CertificateTypeClient], d.endpoints.NetworkCert(), trustCACertificates)
+		if checkCertificateSignature {
+			trusted, _, username := util.CheckCASignature(*i, networkCert)
+			if !trusted {
+				return false, "", "", nil
+			} else if trustCACertificates {
+				return true, username, "tls", nil
+			}
+		}
+
+		trusted, username := util.CheckMutualTLS(*i, trustedCerts[db.CertificateTypeClient])
 		if trusted {
 			return true, username, "tls", nil
 		}

lxd/util/http.go

@@ -21,6 +21,7 @@ import (
 	log "gopkg.in/inconshreveable/log15.v2"
 
 	"github.com/canonical/lxd/shared"
+	"github.com/canonical/lxd/shared/api"
 	"github.com/canonical/lxd/shared/logger"
 )
 
@@ -148,37 +149,72 @@ type ContextAwareRequest interface {
 	WithContext(ctx context.Context) *http.Request
 }
 
-// CheckTrustState checks whether the given client certificate is trusted
-// (i.e. it has a valid time span and it belongs to the given list of trusted
-// certificates).
-// Returns whether or not the certificate is trusted, and the fingerprint of the certificate.
-func CheckTrustState(cert x509.Certificate, trustedCerts map[string]x509.Certificate, networkCert *shared.CertInfo, trustCACertificates bool) (bool, string) {
-	// Extra validity check (should have been caught by TLS stack)
-	if time.Now().Before(cert.NotBefore) || time.Now().After(cert.NotAfter) {
-		return false, ""
+// certificateInDate returns an error if the current time is before the certificates "not before", or after the
+// certificates "not after".
+func certificateInDate(cert x509.Certificate) error {
+	now := time.Now()
+	if now.Before(cert.NotBefore) {
+		return api.StatusErrorf(http.StatusUnauthorized, "Certificate is not yet valid")
 	}
 
-	if networkCert != nil && trustCACertificates {
-		ca := networkCert.CA()
+	if now.After(cert.NotAfter) {
+		return api.StatusErrorf(http.StatusUnauthorized, "Certificate has expired")
+	}
 
-		if ca != nil && cert.CheckSignatureFrom(ca) == nil {
-			// Check whether the certificate has been revoked.
-			crl := networkCert.CRL()
+	return nil
+}
 
-			if crl != nil {
-				for _, revoked := range crl.TBSCertList.RevokedCertificates {
-					if cert.SerialNumber.Cmp(revoked.SerialNumber) == 0 {
-						return false, "" // Certificate is revoked, so not trusted anymore.
-					}
-				}
-			}
+// CheckCASignature returns whether the certificate is signed by the CA, whether the certificate has been revoked, and the
+// certificate fingerprint.
+func CheckCASignature(cert x509.Certificate, networkCert *shared.CertInfo) (trusted bool, revoked bool, fingerprint string) {
+	err := certificateInDate(cert)
+	if err != nil {
+		return false, false, ""
+	}
+
+	if networkCert == nil {
+		logger.Error("Failed to check certificate has been signed by the CA, no network certificate provided")
+		return false, false, ""
+	}
+
+	ca := networkCert.CA()
+	if ca == nil {
+		logger.Error("Failed to check certificate has been signed by the CA, no CA defined on network certificate")
+		return false, false, ""
+	}
 
-			// Certificate not revoked, so trust it as is signed by CA cert.
-			return true, shared.CertFingerprint(&cert)
+	err = cert.CheckSignatureFrom(ca)
+	if err != nil {
+		// Certificate not signed by CA.
+		return false, false, ""
+	}
+
+	crl := networkCert.CRL()
+	if crl == nil {
+		// No revokation list entries to check.
+		return true, false, shared.CertFingerprint(&cert)
+	}
+
+	for _, revoked := range crl.TBSCertList.RevokedCertificates {
+		if cert.SerialNumber.Cmp(revoked.SerialNumber) == 0 {
+			// Certificate has been revoked
+			return false, true, ""
 		}
 	}
 
-	// Check whether client certificate is in trust store.
+	// Certificate not revoked.
+	return true, false, shared.CertFingerprint(&cert)
+}
+
+// CheckMutualTLS checks whether the given certificate is valid and is present in the given trustedCerts map.
+// Returns true if the certificate is trusted, and the fingerprint of the certificate.
+func CheckMutualTLS(cert x509.Certificate, trustedCerts map[string]x509.Certificate) (bool, string) {
+	err := certificateInDate(cert)
+	if err != nil {
+		return false, ""
+	}
+
+	// Check whether client certificate is in the map of trusted certs.
 	for fingerprint, v := range trustedCerts {
 		if bytes.Compare(cert.Raw, v.Raw) == 0 {
 			logger.Debug("Matched trusted cert", log.Ctx{"fingerprint": fingerprint, "subject": v.Subject})

lxd/util/net.go

@@ -111,7 +111,7 @@ func ServerTLSConfig(cert *shared.CertInfo) *tls.Config {
 		config.RootCAs = pool
 		config.ClientCAs = pool
 
-		logger.Infof("LXD is in CA mode, only CA-signed certificates will be allowed")
+		logger.Infof("LXD is in CA mode, only CA-signed client certificates will be allowed")
 	}
 
 	config.BuildNameToCertificate()

shared/network.go

@@ -147,6 +147,12 @@ func GetTLSConfigMem(tlsClientCert string, tlsClientKey string, tlsClientCA stri
 		}
 
 		tlsConfig.Certificates = []tls.Certificate{cert}
+		tlsConfig.GetClientCertificate = func(info *tls.CertificateRequestInfo) (*tls.Certificate, error) {
+			// GetClientCertificate is called if not nil instead of performing the default selection of an appropriate
+			// certificate from the `Certificates` list. We only have one-key pair to send, and we always want to send it
+			// because this is what uniquely identifies the caller to the server.
+			return &cert, nil
+		}
 	}
 
 	var tlsRemoteCert *x509.Certificate