Merge pull request #1458 from stgraber/tls

Fix gap in validation of pre-existing certificates when switching to PKI mode

cmd/incusd/certificates.go

@@ -177,7 +177,11 @@ func certificatesGet(d *Daemon, r *http.Request) response.Response {
 
 	body := []string{}
 
-	trustedCertificates := d.getTrustedCertificates()
+	trustedCertificates, err := d.getTrustedCertificates()
+	if err != nil {
+		return response.SmartError(err)
+	}
+
 	for _, certs := range trustedCertificates {
 		for _, cert := range certs {
 			fingerprint := localtls.CertFingerprint(&cert)

cmd/incusd/daemon.go

@@ -429,8 +429,44 @@ func (d *Daemon) checkTrustedClient(r *http.Request) error {
 }
 
 // getTrustedCertificates returns trusted certificates key on DB type and fingerprint.
-func (d *Daemon) getTrustedCertificates() map[certificate.Type]map[string]x509.Certificate {
-	return d.clientCerts.GetCertificates()
+//
+// When in PKI mode, this also filters out any non-server certificate which isn't issued by the PKI.
+func (d *Daemon) getTrustedCertificates() (map[certificate.Type]map[string]x509.Certificate, error) {
+	certs := d.clientCerts.GetCertificates()
+
+	// If not in PKI mode, return all certificates.
+	if !util.PathExists(internalUtil.VarPath("server.ca")) {
+		return certs, nil
+	}
+
+	// If in PKI mode, filter certificates that aren't trusted by the CA.
+	ca, err := localtls.ReadCert(internalUtil.VarPath("server.ca"))
+	if err != nil {
+		return nil, err
+	}
+
+	certPool := x509.NewCertPool()
+	certPool.AddCert(ca)
+
+	for certType, certEntries := range certs {
+		if certType == certificate.TypeServer {
+			continue
+		}
+
+		for name, entry := range certEntries {
+			_, err := entry.Verify(x509.VerifyOptions{
+				Roots:     certPool,
+				KeyUsages: []x509.ExtKeyUsage{x509.ExtKeyUsageClientAuth},
+			})
+
+			if err != nil {
+				// Skip certificates that aren't signed by the PKI.
+				delete(certs[certType], name)
+			}
+		}
+	}
+
+	return certs, nil
 }
 
 // Authenticate validates an incoming http Request
@@ -441,7 +477,10 @@ func (d *Daemon) getTrustedCertificates() map[certificate.Type]map[string]x509.C
 // Returns whether trusted or not, the username (or certificate fingerprint) of the trusted client, and the type of
 // client that has been authenticated (cluster, unix, or tls).
 func (d *Daemon) Authenticate(w http.ResponseWriter, r *http.Request) (bool, string, string, error) {
-	trustedCerts := d.getTrustedCertificates()
+	trustedCerts, err := d.getTrustedCertificates()
+	if err != nil {
+		return false, "", "", err
+	}
 
 	// Allow internal cluster traffic by checking against the trusted certfificates.
 	if r.TLS != nil {

internal/server/cluster/gateway.go

@@ -154,10 +154,18 @@ func setDqliteVersionHeader(request *http.Request) {
 // These handlers might return 404, either because this server is a
 // non-clustered member not available over the network or because it is not a
 // database node part of the dqlite cluster.
-func (g *Gateway) HandlerFuncs(heartbeatHandler HeartbeatHandler, trustedCerts func() map[certificate.Type]map[string]x509.Certificate) map[string]http.HandlerFunc {
+func (g *Gateway) HandlerFuncs(heartbeatHandler HeartbeatHandler, trustedCerts func() (map[certificate.Type]map[string]x509.Certificate, error)) map[string]http.HandlerFunc {
 	database := func(w http.ResponseWriter, r *http.Request) {
 		g.lock.RLock()
-		if !tlsCheckCert(r, g.networkCert, g.state().ServerCert(), trustedCerts()) {
+
+		certs, err := trustedCerts()
+		if err != nil {
+			g.lock.RUnlock()
+			http.Error(w, "403 invalid client certificate", http.StatusForbidden)
+			return
+		}
+
+		if !tlsCheckCert(r, g.networkCert, g.state().ServerCert(), certs) {
 			g.lock.RUnlock()
 			http.Error(w, "403 invalid client certificate", http.StatusForbidden)
 			return

internal/server/cluster/gateway_test.go

@@ -22,6 +22,10 @@ import (
 	localtls "github.com/lxc/incus/v6/shared/tls"
 )
 
+func trustedCerts() (map[certificate.Type]map[string]x509.Certificate, error) {
+	return nil, nil
+}
+
 // Basic creation and shutdown. By default, the gateway runs an in-memory gRPC
 // server.
 func TestGateway_Single(t *testing.T) {
@@ -37,10 +41,6 @@ func TestGateway_Single(t *testing.T) {
 	gateway := newGateway(t, node, cert, s)
 	defer func() { _ = gateway.Shutdown() }()
 
-	trustedCerts := func() map[certificate.Type]map[string]x509.Certificate {
-		return nil
-	}
-
 	handlerFuncs := gateway.HandlerFuncs(nil, trustedCerts)
 	assert.Len(t, handlerFuncs, 1)
 	for endpoint, f := range handlerFuncs {
@@ -101,10 +101,6 @@ func TestGateway_SingleWithNetworkAddress(t *testing.T) {
 	gateway := newGateway(t, node, cert, s)
 	defer func() { _ = gateway.Shutdown() }()
 
-	trustedCerts := func() map[certificate.Type]map[string]x509.Certificate {
-		return nil
-	}
-
 	for path, handler := range gateway.HandlerFuncs(nil, trustedCerts) {
 		mux.HandleFunc(path, handler)
 	}
@@ -146,10 +142,6 @@ func TestGateway_NetworkAuth(t *testing.T) {
 	gateway := newGateway(t, node, cert, s)
 	defer func() { _ = gateway.Shutdown() }()
 
-	trustedCerts := func() map[certificate.Type]map[string]x509.Certificate {
-		return nil
-	}
-
 	for path, handler := range gateway.HandlerFuncs(nil, trustedCerts) {
 		mux.HandleFunc(path, handler)
 	}

internal/server/cluster/heartbeat_test.go

@@ -2,7 +2,6 @@ package cluster_test
 
 import (
 	"context"
-	"crypto/x509"
 	"net/http"
 	"net/http/httptest"
 	"testing"
@@ -12,7 +11,6 @@ import (
 	"github.com/stretchr/testify/assert"
 	"github.com/stretchr/testify/require"
 
-	"github.com/lxc/incus/v6/internal/server/certificate"
 	"github.com/lxc/incus/v6/internal/server/cluster"
 	clusterConfig "github.com/lxc/incus/v6/internal/server/cluster/config"
 	"github.com/lxc/incus/v6/internal/server/db"
@@ -214,10 +212,6 @@ func (f *heartbeatFixture) node() (*state.State, *cluster.Gateway, string) {
 	mux := http.NewServeMux()
 	server := newServer(serverCert, mux)
 
-	trustedCerts := func() map[certificate.Type]map[string]x509.Certificate {
-		return nil
-	}
-
 	for path, handler := range gateway.HandlerFuncs(nil, trustedCerts) {
 		mux.HandleFunc(path, handler)
 	}

internal/server/cluster/membership_test.go

@@ -139,10 +139,6 @@ func TestBootstrap(t *testing.T) {
 	// The cluster certificate is in place.
 	assert.True(t, util.PathExists(filepath.Join(state.OS.VarDir, "cluster.crt")))
 
-	trustedCerts := func() map[certificate.Type]map[string]x509.Certificate {
-		return nil
-	}
-
 	// The dqlite driver is now exposed over the network.
 	for path, handler := range gateway.HandlerFuncs(nil, trustedCerts) {
 		mux.HandleFunc(path, handler)
@@ -297,12 +293,12 @@ func TestJoin(t *testing.T) {
 	altServerCert := localtls.TestingAltKeyPair()
 	trustedAltServerCert, _ := x509.ParseCertificate(altServerCert.KeyPair().Certificate[0])
 
-	trustedCerts := func() map[certificate.Type]map[string]x509.Certificate {
+	trustedCerts := func() (map[certificate.Type]map[string]x509.Certificate, error) {
 		return map[certificate.Type]map[string]x509.Certificate{
 			certificate.TypeServer: {
 				altServerCert.Fingerprint(): *trustedAltServerCert,
 			},
-		}
+		}, nil
 	}
 
 	for path, handler := range targetGateway.HandlerFuncs(nil, trustedCerts) {

internal/server/cluster/upgrade_test.go

@@ -2,7 +2,6 @@ package cluster_test
 
 import (
 	"context"
-	"crypto/x509"
 	"errors"
 	"fmt"
 	"io/fs"
@@ -18,7 +17,6 @@ import (
 	"github.com/stretchr/testify/assert"
 	"github.com/stretchr/testify/require"
 
-	"github.com/lxc/incus/v6/internal/server/certificate"
 	"github.com/lxc/incus/v6/internal/server/cluster"
 	"github.com/lxc/incus/v6/internal/server/db"
 	"github.com/lxc/incus/v6/internal/server/node"
@@ -160,10 +158,6 @@ func TestUpgradeMembersWithoutRole(t *testing.T) {
 	gateway := newGateway(t, state.DB.Node, serverCert, state)
 	defer func() { _ = gateway.Shutdown() }()
 
-	trustedCerts := func() map[certificate.Type]map[string]x509.Certificate {
-		return nil
-	}
-
 	for path, handler := range gateway.HandlerFuncs(nil, trustedCerts) {
 		mux.HandleFunc(path, handler)
 	}

test/suites/pki.sh

@@ -36,20 +36,35 @@ test_pki() {
     fi
   )
 
-  # Setup the daemon.
+  # Setup the daemon in normal mode
   INCUS5_DIR=$(mktemp -d -p "${TEST_DIR}" XXX)
   chmod +x "${INCUS5_DIR}"
-  cp "${TEST_DIR}/pki/keys/ca.crt" "${INCUS5_DIR}/server.ca"
-  cp "${TEST_DIR}/pki/keys/crl.pem" "${INCUS5_DIR}/ca.crl"
   spawn_incus "${INCUS5_DIR}" true
   INCUS5_ADDR=$(cat "${INCUS5_DIR}/incus.addr")
 
+  # Generate, trust and test a client certificate
+  openssl req -x509 -newkey rsa:4096 -sha384 -keyout "${INCUS_CONF}/simple-client.key" -nodes -out "${INCUS_CONF}/simple-client.crt" -days 1 -subj "/CN=test.local"
+  INCUS_DIR="${INCUS5_DIR}" incus config trust add-certificate "${INCUS_CONF}/simple-client.crt"
+  INCUS_DIR="${INCUS5_DIR}" incus config set user.test foo
+  curl -k -s --cert "${INCUS_CONF}/simple-client.crt" --key "${INCUS_CONF}/simple-client.key" "https://${INCUS5_ADDR}/1.0" | grep -q "user.test.*foo" || false
+
+  # Restart the daemon in PKI mode
+  shutdown_incus "${INCUS5_DIR}"
+  cp "${TEST_DIR}/pki/keys/ca.crt" "${INCUS5_DIR}/server.ca"
+  cp "${TEST_DIR}/pki/keys/crl.pem" "${INCUS5_DIR}/ca.crl"
+  respawn_incus "${INCUS5_DIR}" true
+
   # Setup the client.
   INC5_DIR=$(mktemp -d -p "${TEST_DIR}" XXX)
   cp "${TEST_DIR}/pki/keys/incus-client.crt" "${INC5_DIR}/client.crt"
   cp "${TEST_DIR}/pki/keys/incus-client.key" "${INC5_DIR}/client.key"
   cp "${TEST_DIR}/pki/keys/ca.crt" "${INC5_DIR}/client.ca"
 
+  # Re-test the regular client certificate
+  curl -k -s --cert "${INCUS_CONF}/simple-client.crt" --key "${INCUS_CONF}/simple-client.key" "https://${INCUS5_ADDR}/1.0" incus/1.0 | grep -q "user.test.*foo" && false
+  fingerprint="$(INCUS_DIR="${INCUS5_DIR}" incus config trust list -cf -fcsv)"
+  INCUS_DIR="${INCUS5_DIR}" incus config trust remove "${fingerprint}"
+
   # Confirm that a valid client certificate works.
   (
     set -e