Address comments to use context for ending goroutines

Signed-off-by: Guilherme Carvalho <[email protected]>

Author: guilhermocc

support/oidc-discovery-provider/README.md

@@ -177,7 +177,7 @@ workload_api {
 ```hcl
 log_level = "debug"
 domains = ["mypublicdomain.test"]
-serving_certificate {
+serving_cert_file {
  cert_file_path = "/some/path/on/disk/to/cert.pem"
  key_file_path = "/some/path/on/disk/to/key.pem"
 }
@@ -191,7 +191,7 @@ server_api {
 ```hcl
 log_level = "debug"
 domains = ["mypublicdomain.test"]
-serving_certificate {
+serving_cert_file {
  cert_file_path = "/some/path/on/disk/to/cert.pem"
  key_file_path = "/some/path/on/disk/to/key.pem"
 }

support/oidc-discovery-provider/cert_manager.go

@@ -1,18 +1,17 @@
 package main
 
 import (
+	"context"
 	"crypto/tls"
 	"crypto/x509"
 	"errors"
 	"fmt"
 	"io/fs"
 	"os"
-	"strings"
 	"sync"
 	"time"
 
 	"github.com/sirupsen/logrus"
-	"golang.org/x/net/idna"
 )
 
 // DiskCertManager is a certificate manager that loads certificates from disk, and watches for changes.
@@ -43,8 +42,6 @@ func NewDiskCertManager(config *ServingCertFileConfig, log logrus.FieldLogger) (
 		return nil, fmt.Errorf("failed to load certificate: %w", err)
 	}
 
-	go dm.watchFileChanges()
-
 	return dm, nil
 }
 
@@ -61,46 +58,27 @@ func (m *DiskCertManager) TLSConfig() *tls.Config {
 
 // getCertificate is called by the TLS stack when a new TLS connection is established.
 func (m *DiskCertManager) getCertificate(chInfo *tls.ClientHelloInfo) (*tls.Certificate, error) {
-	name := chInfo.ServerName
-	if name == "" {
-		return nil, errors.New("missing server name")
-	}
-	if !strings.Contains(strings.Trim(name, "."), ".") {
-		return nil, errors.New("server name component count invalid")
-	}
-
-	// Note that this conversion is necessary because some server names in the handshakes
-	// started by some clients (such as cURL) are not converted to Punycode, which will
-	// prevent us from obtaining certificates for them. In addition, we should also treat
-	// example.com and EXAMPLE.COM as equivalent and return the same certificate for them.
-	// Fortunately, this conversion also helped us deal with this kind of mixedcase problems.
-	//
-	// Due to the "σςΣ" problem (see https://unicode.org/faq/idn.html#22), we can't use
-	// idna.Punycode.ToASCII (or just idna.ToASCII) here.
-	name, err := idna.Lookup.ToASCII(name)
-	if err != nil {
-		return nil, errors.New("server name contains invalid character")
-	}
-
 	m.certMtx.RLock()
 	defer m.certMtx.RUnlock()
 	cert := m.cert
 
-	// Verify that the certificate is valid for the requested server name.
-	if name != cert.Leaf.Subject.CommonName {
-		if err := cert.Leaf.VerifyHostname(name); err != nil {
-			return nil, fmt.Errorf("server name mismatch: %w", err)
-		}
-	}
-
 	return cert, nil
 }
 
-// watchFileChanges starts a file watcher to watch for changes to the cert and key files.
-func (m *DiskCertManager) watchFileChanges() {
+// WatchFileChanges starts a file watcher to watch for changes to the cert and key files.
+func (m *DiskCertManager) WatchFileChanges(ctx context.Context) {
+	m.log.WithField("interval", m.fileSyncInterval).Info("Started watching certificate files")
+
 	ticker := time.NewTicker(m.fileSyncInterval)
-	for range ticker.C {
-		m.syncCertificateFiles()
+	defer ticker.Stop()
+	for {
+		select {
+		case <-ctx.Done():
+			m.log.Info("Stopping file watcher")
+			return
+		case <-ticker.C:
+			m.syncCertificateFiles()
+		}
 	}
 }
 

support/oidc-discovery-provider/cert_manager_test_posix.go renamed to support/oidc-discovery-provider/cert_manager_posix_test.go

@@ -20,8 +20,6 @@ var (
 func writeFile(t *testing.T, name string, data []byte) {
 	err := os.WriteFile(name, data, 0600)
 	require.NoError(t, err)
-	_, err = os.Stat(name)
-	require.NoError(t, err)
 }
 
 func removeFile(t *testing.T, name string) {

support/oidc-discovery-provider/cert_manager_test.go

@@ -1,6 +1,7 @@
 package main
 
 import (
+	"context"
 	"crypto/tls"
 	"crypto/x509"
 	"crypto/x509/pkix"
@@ -82,13 +83,16 @@ func TestTLSConfig(t *testing.T) {
 		ServerName: "oidc-provider-discovery.example.com",
 	}
 
+	ctx, cancelFn := context.WithCancel(context.Background())
 	certManager, err := NewDiskCertManager(&ServingCertFileConfig{
 		CertFilePath:     tmpDir + certFilePath,
 		KeyFilePath:      tmpDir + keyFilePath,
 		FileSyncInterval: 10 * time.Millisecond,
 	}, logger)
 	require.NoError(t, err)
 
+	certManager.WatchFileChanges(ctx)
+
 	tlsConfig := certManager.TLSConfig()
 
 	t.Run("error when configuration does not contain serving cert file settings", func(t *testing.T) {
@@ -150,32 +154,6 @@ func TestTLSConfig(t *testing.T) {
 		require.EqualError(t, err, "failed to load certificate: tls: failed to find any PEM data in key input")
 	})
 
-	t.Run("error when client misses server name", func(t *testing.T) {
-		_, err := tlsConfig.GetCertificate(&tls.ClientHelloInfo{})
-		require.EqualError(t, err, "missing server name")
-	})
-
-	t.Run("error when client send server name with invalid character", func(t *testing.T) {
-		_, err := tlsConfig.GetCertificate(&tls.ClientHelloInfo{
-			ServerName: "example.com:8080",
-		})
-		require.EqualError(t, err, "server name contains invalid character")
-	})
-
-	t.Run("error when client send server name with invalid component count", func(t *testing.T) {
-		_, err := tlsConfig.GetCertificate(&tls.ClientHelloInfo{
-			ServerName: "example",
-		})
-		require.EqualError(t, err, "server name component count invalid")
-	})
-
-	t.Run("error when client send wrong server name", func(t *testing.T) {
-		_, err := tlsConfig.GetCertificate(&tls.ClientHelloInfo{
-			ServerName: "example.com",
-		})
-		require.EqualError(t, err, `server name mismatch: x509: certificate is not valid for any names, but wanted to match example.com`)
-	})
-
 	t.Run("success loading initial certificate from disk", func(t *testing.T) {
 		cert, err := tlsConfig.GetCertificate(chInfo)
 		require.NoError(t, err)
@@ -398,4 +376,13 @@ func TestTLSConfig(t *testing.T) {
 			return reflect.DeepEqual(oidcServerCertUpdated3, x509Cert)
 		}, 10*time.Second, 100*time.Millisecond, "Failed to assert updated certificate")
 	})
+
+	t.Run("stop file watcher when context is canceled", func(t *testing.T) {
+		cancelFn()
+
+		require.Eventuallyf(t, func() bool {
+			lastEntry := logHook.LastEntry()
+			return lastEntry.Level == logrus.InfoLevel && lastEntry.Message == "Stopping file watcher"
+		}, 10*time.Second, 10*time.Millisecond, "Failed to assert file watcher stop log")
+	})
 }

support/oidc-discovery-provider/cert_manager_test_windows.go renamed to support/oidc-discovery-provider/cert_manager_windows_test.go

@@ -1,12 +1,15 @@
 package main
 
 import (
+	"context"
 	"crypto/tls"
 	"flag"
 	"fmt"
 	"net"
 	"net/http"
 	"os"
+	"os/signal"
+	"syscall"
 	"time"
 
 	"github.com/sirupsen/logrus"
@@ -49,6 +52,9 @@ func run(configPath string) error {
 	}
 	defer log.Close()
 
+	ctx, stop := signal.NotifyContext(context.Background(), syscall.SIGINT, syscall.SIGTERM)
+	defer stop()
+
 	source, err := newSource(log, config)
 	if err != nil {
 		return err
@@ -66,7 +72,7 @@ func run(configPath string) error {
 		handler = logHandler(log, handler)
 	}
 
-	listener, err := buildNetListener(config, log)
+	listener, err := buildNetListener(ctx, config, log)
 	if err != nil {
 		return err
 	}
@@ -91,10 +97,19 @@ func run(configPath string) error {
 		Handler:           handler,
 		ReadHeaderTimeout: 10 * time.Second,
 	}
+
+	go func() {
+		<-ctx.Done()
+		err = server.Shutdown(context.Background())
+		if err != nil {
+			log.Error(err)
+		}
+	}()
+
 	return server.Serve(listener)
 }
 
-func buildNetListener(config *Config, log *log.Logger) (listener net.Listener, err error) {
+func buildNetListener(ctx context.Context, config *Config, log *log.Logger) (listener net.Listener, err error) {
 	switch {
 	case config.InsecureAddr != "":
 		listener, err = net.Listen("tcp", config.InsecureAddr)
@@ -112,7 +127,7 @@ func buildNetListener(config *Config, log *log.Logger) (listener net.Listener, e
 			telemetry.Address: listener.Addr().String(),
 		}).Info("Serving HTTP")
 	case config.ServingCertFile != nil:
-		listener, err = newListenerWithServingCert(log, config)
+		listener, err = newListenerWithServingCert(ctx, log, config)
 		if err != nil {
 			return nil, err
 		}
@@ -152,15 +167,18 @@ func newSource(log logrus.FieldLogger, config *Config) (JWKSSource, error) {
 	}
 }
 
-func newListenerWithServingCert(log logrus.FieldLogger, config *Config) (net.Listener, error) {
+func newListenerWithServingCert(ctx context.Context, log logrus.FieldLogger, config *Config) (net.Listener, error) {
 	certManager, err := NewDiskCertManager(config.ServingCertFile, log)
 	if err != nil {
 		return nil, err
 	}
+	go func() {
+		certManager.WatchFileChanges(ctx)
+	}()
 
 	tlsConfig := certManager.TLSConfig()
 
-	tcpListener, err := net.ListenTCP("tcp", &net.TCPAddr{Port: 443})
+	tcpListener, err := net.ListenTCP("tcp", &net.TCPAddr{Port: 8080})
 	if err != nil {
 		return nil, fmt.Errorf("failed to create listener using certificate from disk: %w", err)
 	}