Fix CRL for client certificates

cmd/integration_test.go

@@ -18,6 +18,7 @@ import (
 	"net/http/httptest"
 	"net/url"
 	"os"
+	"os/exec"
 	"testing"
 	"time"
 
@@ -648,7 +649,7 @@ func startSmokescreen(t *testing.T, useTLS bool, logHook logrus.Hook, httpProxyA
 		)
 	}
 
-	if httpProxyAddr != ""{
+	if httpProxyAddr != "" {
 		args = append(args, fmt.Sprintf("--upstream-http-proxy-addr=%s", httpProxyAddr))
 		args = append(args, fmt.Sprintf("--upstream-https-proxy-addr=%s", httpProxyAddr))
 	}
@@ -683,3 +684,228 @@ func startSmokescreen(t *testing.T, useTLS bool, logHook logrus.Hook, httpProxyA
 
 	return conf, server, nil
 }
+
+func TestCRLValidation(t *testing.T) {
+	cleanup := generateTestPKI(t)
+	defer cleanup()
+
+	cleanupACL := generatePermissiveACL(t)
+	defer cleanupACL()
+
+	// Start smokescreen with TLS and CRL configuration
+	args := []string{
+		"smokescreen",
+		"--listen-ip=127.0.0.1",
+		"--egress-acl-file=testdata/pki-crl/acl.yaml",
+		"--tls-crl-file=testdata/pki-crl/crl.pem",
+		"--tls-server-bundle-file=testdata/pki-crl/server-bundle.pem",
+		"--tls-client-ca-file=testdata/pki-crl/ca.pem",
+	}
+	conf, err := NewConfiguration(args, nil)
+	require.NoError(t, err)
+
+	// Add role from request function
+	conf.RoleFromRequest = testRFRCert
+	conf.MetricsClient = metrics.NewNoOpMetricsClient()
+
+	handler := smokescreen.BuildProxy(conf)
+	server := httptest.NewUnstartedServer(handler)
+	server.TLS = conf.TlsConfig
+	server.StartTLS()
+	defer server.Close()
+
+	// Test cases for valid and revoked certificates
+	testCases := []struct {
+		name          string
+		certFile      string
+		keyFile       string
+		expectSuccess bool
+	}{
+		{
+			name:          "Valid certificate",
+			certFile:      "testdata/pki-crl/valid-client.pem",
+			keyFile:       "testdata/pki-crl/valid-client-key.pem",
+			expectSuccess: true,
+		},
+		{
+			name:          "Revoked certificate",
+			certFile:      "testdata/pki-crl/revoked-client.pem",
+			keyFile:       "testdata/pki-crl/revoked-client-key.pem",
+			expectSuccess: false,
+		},
+	}
+
+	for _, tc := range testCases {
+		t.Run(tc.name, func(t *testing.T) {
+			cert, err := tls.LoadX509KeyPair(tc.certFile, tc.keyFile)
+			require.NoError(t, err)
+
+			caCert, err := os.ReadFile("testdata/pki-crl/ca.pem")
+			require.NoError(t, err)
+			caCertPool := x509.NewCertPool()
+			caCertPool.AppendCertsFromPEM(caCert)
+
+			proxyURL, err := url.Parse(server.URL)
+			require.NoError(t, err)
+
+			client := &http.Client{
+				Transport: &http.Transport{
+					TLSClientConfig: &tls.Config{
+						Certificates:       []tls.Certificate{cert},
+						RootCAs:            caCertPool,
+						InsecureSkipVerify: true, // For testing only - allows connecting to proxy with self-signed cert
+					},
+					Proxy: http.ProxyURL(proxyURL),
+				},
+			}
+
+			// Make request through proxy
+			resp, err := client.Get("https://example.com")
+
+			if tc.expectSuccess {
+				require.NoError(t, err)
+				require.Equal(t, http.StatusOK, resp.StatusCode)
+			} else {
+				require.Error(t, err)
+				var netErr *net.OpError
+				require.True(t, errors.As(err, &netErr))
+			}
+		})
+	}
+}
+
+func generateTestPKI(t *testing.T) func() {
+	// Create test directory
+	err := os.MkdirAll("testdata/pki-crl", 0755)
+	require.NoError(t, err)
+
+	// Helper function to run openssl commands
+	runOpenSSL := func(args ...string) {
+		cmd := exec.Command("openssl", args...)
+		output, err := cmd.CombinedOutput()
+		if err != nil {
+			t.Fatalf("openssl %v failed: %v\nOutput: %s", args, err, output)
+		}
+	}
+
+	// Generate CA key and certificate
+	runOpenSSL("genrsa", "-out", "testdata/pki-crl/ca-key.pem", "2048")
+	runOpenSSL("req", "-new", "-x509", "-days", "365", "-key", "testdata/pki-crl/ca-key.pem",
+		"-out", "testdata/pki-crl/ca.pem", "-subj", "/CN=Test CA")
+
+	// Generate server key and certificate
+	runOpenSSL("genrsa", "-out", "testdata/pki-crl/server-key.pem", "2048")
+	runOpenSSL("req", "-new", "-key", "testdata/pki-crl/server-key.pem",
+		"-out", "testdata/pki-crl/server.csr", "-subj", "/CN=localhost")
+	runOpenSSL("x509", "-req", "-days", "365", "-in", "testdata/pki-crl/server.csr",
+		"-CA", "testdata/pki-crl/ca.pem", "-CAkey", "testdata/pki-crl/ca-key.pem",
+		"-CAcreateserial", "-out", "testdata/pki-crl/server.pem")
+
+	// Create server bundle by concatenating key and cert
+	serverKey, err := os.ReadFile("testdata/pki-crl/server-key.pem")
+	require.NoError(t, err)
+	serverCert, err := os.ReadFile("testdata/pki-crl/server.pem")
+	require.NoError(t, err)
+	err = os.WriteFile("testdata/pki-crl/server-bundle.pem",
+		append(serverKey, serverCert...), 0644)
+	require.NoError(t, err)
+
+	// Generate valid client key and certificate
+	runOpenSSL("genrsa", "-out", "testdata/pki-crl/valid-client-key.pem", "2048")
+	runOpenSSL("req", "-new", "-key", "testdata/pki-crl/valid-client-key.pem",
+		"-out", "testdata/pki-crl/valid-client.csr", "-subj", "/CN=valid-client")
+	runOpenSSL("x509", "-req", "-days", "365", "-in", "testdata/pki-crl/valid-client.csr",
+		"-CA", "testdata/pki-crl/ca.pem", "-CAkey", "testdata/pki-crl/ca-key.pem",
+		"-CAcreateserial", "-out", "testdata/pki-crl/valid-client.pem")
+
+	// Generate to-be-revoked client key and certificate
+	runOpenSSL("genrsa", "-out", "testdata/pki-crl/revoked-client-key.pem", "2048")
+	runOpenSSL("req", "-new", "-key", "testdata/pki-crl/revoked-client-key.pem",
+		"-out", "testdata/pki-crl/revoked-client.csr", "-subj", "/CN=revoked-client")
+	runOpenSSL("x509", "-req", "-days", "365", "-in", "testdata/pki-crl/revoked-client.csr",
+		"-CA", "testdata/pki-crl/ca.pem", "-CAkey", "testdata/pki-crl/ca-key.pem",
+		"-CAcreateserial", "-out", "testdata/pki-crl/revoked-client.pem")
+
+	// Create index.txt and crlnumber for CRL generation
+	err = os.WriteFile("testdata/pki-crl/index.txt", []byte{}, 0644)
+	require.NoError(t, err)
+	err = os.WriteFile("testdata/pki-crl/crlnumber", []byte("01\n"), 0644)
+	require.NoError(t, err)
+
+	// Create openssl CA config with extensions
+	caConfig := `
+[ ca ]
+default_ca = CA_default
+
+[ CA_default ]
+database = testdata/pki-crl/index.txt
+crlnumber = testdata/pki-crl/crlnumber
+default_days = 365
+default_crl_days = 30
+default_md = sha256
+copy_extensions = copy
+crl_extensions = crl_ext
+
+[ crl_ext ]
+authorityKeyIdentifier=keyid:always,issuer:always
+`
+	err = os.WriteFile("testdata/pki-crl/ca-config.conf", []byte(caConfig), 0644)
+	require.NoError(t, err)
+
+	// Generate initial CRL
+	runOpenSSL("ca", "-gencrl",
+		"-keyfile", "testdata/pki-crl/ca-key.pem",
+		"-cert", "testdata/pki-crl/ca.pem",
+		"-out", "testdata/pki-crl/crl.pem",
+		"-config", "testdata/pki-crl/ca-config.conf")
+
+	// Revoke the second certificate
+	runOpenSSL("ca", "-revoke", "testdata/pki-crl/revoked-client.pem",
+		"-keyfile", "testdata/pki-crl/ca-key.pem",
+		"-cert", "testdata/pki-crl/ca.pem",
+		"-config", "testdata/pki-crl/ca-config.conf")
+
+	// Generate updated CRL with the revoked certificate
+	runOpenSSL("ca", "-gencrl",
+		"-keyfile", "testdata/pki-crl/ca-key.pem",
+		"-cert", "testdata/pki-crl/ca.pem",
+		"-out", "testdata/pki-crl/crl.pem",
+		"-config", "testdata/pki-crl/ca-config.conf")
+
+	// Return cleanup function
+	return func() {
+		err := os.RemoveAll("testdata/pki-crl")
+		if err != nil {
+			t.Logf("Failed to cleanup test PKI: %v", err)
+		}
+	}
+}
+
+func generatePermissiveACL(t *testing.T) func() {
+	aclConfig := `---
+version: v1
+services:
+  - name: "*"
+    project: test
+    action: open
+    allowed_domains:
+      - "*.example.com"
+
+default:
+  name: unknown-role
+  project: security
+  action: open
+`
+	err := os.MkdirAll("testdata/pki-crl", 0755)
+	require.NoError(t, err)
+
+	err = os.WriteFile("testdata/pki-crl/acl.yaml", []byte(aclConfig), 0644)
+	require.NoError(t, err)
+
+	return func() {
+		err := os.Remove("testdata/pki-crl/acl.yaml")
+		if err != nil {
+			t.Logf("Failed to cleanup ACL config: %v", err)
+		}
+	}
+}

cmd/smokescreen.go

@@ -95,7 +95,7 @@ func NewConfiguration(args []string, logger *log.Logger) (*smokescreen.Config, e
 			Usage: "Expose prometheus metrics on `ENDPOINT`. Requires --expose-prometheus-metrics to be set. Defaults to \"/metrics\"",
 		},
 		cli.StringFlag{
-			Name: "prometheus-listen-ip",
+			Name:  "prometheus-listen-ip",
 			Value: "0.0.0.0",
 			Usage: "Listen for prometheus metrics on interface with address IP. Requires --expose-prometheus-metrics to be set. Defaults to \"0.0.0.0\"",
 		},
@@ -270,12 +270,6 @@ func NewConfiguration(args []string, logger *log.Logger) (*smokescreen.Config, e
 			}
 		}
 
-		if c.IsSet("tls-crl-file") {
-			if err := conf.SetupCrls(c.StringSlice("tls-crl-file")); err != nil {
-				return err
-			}
-		}
-
 		if c.IsSet("unsafe-allow-private-ranges") {
 			conf.UnsafeAllowPrivateRanges = c.Bool("unsafe-allow-private-ranges")
 		}
@@ -297,6 +291,12 @@ func NewConfiguration(args []string, logger *log.Logger) (*smokescreen.Config, e
 			}
 		}
 
+		if c.IsSet("tls-crl-file") {
+			if err := conf.SetupCrls(c.StringSlice("tls-crl-file")); err != nil {
+				return err
+			}
+		}
+
 		if c.IsSet("upstream-http-proxy-addr") {
 			conf.UpstreamHttpProxyAddr = c.String("upstream-http-proxy-addr")
 		}

pkg/smokescreen/config.go

@@ -266,7 +266,7 @@ func NewConfig() *Config {
 
 func (config *Config) SetupCrls(crlFiles []string) error {
 	for _, crlFile := range crlFiles {
-		crlBytes, err := ioutil.ReadFile(crlFile)
+		crlBytes, err := os.ReadFile(crlFile)
 		if err != nil {
 			return err
 		}

pkg/smokescreen/smokescreen.go

@@ -1,8 +1,11 @@
 package smokescreen
 
 import (
+	"bytes"
 	"context"
 	"crypto/tls"
+	"crypto/x509"
+	"encoding/hex"
 	"errors"
 	"fmt"
 	"io"
@@ -448,6 +451,67 @@ func newContext(cfg *Config, proxyType string, req *http.Request) *SmokescreenCo
 func BuildProxy(config *Config) *goproxy.ProxyHttpServer {
 	proxy := goproxy.NewProxyHttpServer(goproxy.WithHttpProxyAddr(config.UpstreamHttpProxyAddr), goproxy.WithHttpsProxyAddr(config.UpstreamHttpsProxyAddr))
 	proxy.Verbose = false
+
+	// Add CRL verification to TLS config if CRL file is provided
+	if config.TlsConfig != nil && config.CrlByAuthorityKeyId != nil {
+		originalVerify := config.TlsConfig.VerifyPeerCertificate
+		config.TlsConfig.VerifyPeerCertificate = func(rawCerts [][]byte, verifiedChains [][]*x509.Certificate) error {
+			config.Log.WithFields(logrus.Fields{
+				"rawCerts":       rawCerts,
+				"verifiedChains": verifiedChains,
+			}).Debug("VerifyPeerCertificate called")
+			// First run the original verification if it exists
+			if originalVerify != nil {
+				if err := originalVerify(rawCerts, verifiedChains); err != nil {
+					config.Log.WithFields(logrus.Fields{
+						"error": err,
+					}).Error("original verification failed")
+					return err
+				}
+			}
+
+			// Then check CRL for each certificate in the chain
+			for _, chain := range verifiedChains {
+				for _, cert := range chain {
+					// Skip root CA as it's typically not in CRLs
+					if cert.IsCA && bytes.Equal(cert.RawSubject, cert.RawIssuer) {
+						config.Log.WithFields(logrus.Fields{
+							"cert": cert.Subject.CommonName,
+						}).Debug("skipping root CA")
+						continue
+					}
+
+					// Get CRL for this certificate's issuer
+					crl := config.CrlByAuthorityKeyId[string(cert.AuthorityKeyId)]
+					if crl == nil {
+						// No CRL for this issuer - could be a warning but let's allow for now
+						config.Log.WithFields(logrus.Fields{
+							"cert":           cert.Subject.CommonName,
+							"authorityKeyId": hex.EncodeToString(cert.AuthorityKeyId),
+						}).Warn("no CRL for certificate")
+						continue
+					}
+
+					// Check if certificate is revoked
+					for _, revoked := range crl.TBSCertList.RevokedCertificates {
+						if cert.SerialNumber.Cmp(revoked.SerialNumber) == 0 {
+							config.Log.WithFields(logrus.Fields{
+								"cert":   cert.Subject.CommonName,
+								"serial": cert.SerialNumber,
+							}).Warn("certificate is revoked")
+							return fmt.Errorf("certificate with serial number %v is revoked", cert.SerialNumber)
+						}
+						config.Log.WithFields(logrus.Fields{
+							"cert":   cert.Subject.CommonName,
+							"serial": cert.SerialNumber,
+						}).Debug("certificate is not revoked")
+					}
+				}
+			}
+			return nil
+		}
+	}
+
 	configureTransport(proxy.Tr, config)
 
 	// dialContext will be invoked for both CONNECT and traditional proxy requests