fix data race in CertificateReloader with RWMutex

- replace global hardcodedRecords/mutex with instance fields
- add initialRecords() factory for fresh record copies per instance
- rename initHardcodedRecords to initNameServers, pass nameservers explicitly
- add TestInstanceIsolation to verify instances don't share state
- fix unassigned err in certs/persistFiles

certs/certs.go

@@ -102,7 +102,7 @@ func persistFiles(certificates *certificate.Resource, certType string) {
 		utils.Logger.Fatal().Err(err).Msgf("Failed to save ./.lego/certs/%s/server.pem", certType)
 	}
 
-	os.WriteFile(fmt.Sprintf("./.lego/certs/%s/server.key", certType), certificates.PrivateKey, 0o644)
+	err = os.WriteFile(fmt.Sprintf("./.lego/certs/%s/server.key", certType), certificates.PrivateKey, 0o644)
 	if err != nil {
 		utils.Logger.Fatal().Err(err).Msgf("Failed to save ./.lego/certs/%s/server.key", certType)
 	}

cmd/root.go

@@ -24,6 +24,8 @@ var command = &cobra.Command{
 		viper.SetEnvPrefix("XIP")
 		viper.AutomaticEnv()
 
+		utils.InitLogger(viper.GetString("log-file"))
+
 		email := viper.GetString("Email")
 		_, err := mail.ParseAddress(email)
 		if err != nil {
@@ -84,6 +86,9 @@ var command = &cobra.Command{
 }
 
 func Execute() {
+	command.Flags().String("log-file", utils.DefaultLogFile, "Path to log file")
+	viper.BindPFlag("log-file", command.Flags().Lookup("log-file"))
+
 	command.Flags().Uint("dns-port", 53, "Port for the DNS server")
 	viper.BindPFlag("dns-port", command.Flags().Lookup("dns-port"))
 

http/server.go

@@ -9,6 +9,7 @@ import (
 	"os"
 	"strconv"
 	"strings"
+	"sync"
 	"time"
 
 	"github.com/urfave/negroni"
@@ -145,6 +146,7 @@ func redirectHttpToHttps() {
 type CertificateReloader struct {
 	CertificateFilePath string
 	KeyFilePath         string
+	mu                  sync.RWMutex
 	certificate         *tls.Certificate
 	lastUpdatedAt       time.Time
 }
@@ -155,7 +157,20 @@ func (cr *CertificateReloader) GetCertificate(*tls.ClientHelloInfo) (*tls.Certif
 		return nil, fmt.Errorf("failed checking key file modification time: %w", err)
 	}
 
-	if cr.certificate == nil || stat.ModTime().After(cr.lastUpdatedAt) {
+	cr.mu.RLock()
+	if cr.certificate != nil && !stat.ModTime().After(cr.lastUpdatedAt) {
+		defer cr.mu.RUnlock()
+		return cr.certificate, nil
+	}
+	cr.mu.RUnlock()
+
+	cr.mu.Lock()
+	defer cr.mu.Unlock()
+
+	if cr.certificate != nil && !stat.ModTime().After(cr.lastUpdatedAt) {
+		return cr.certificate, nil
+	}
+
 	pair, err := tls.LoadX509KeyPair(cr.CertificateFilePath, cr.KeyFilePath)
 	if err != nil {
 		return nil, fmt.Errorf("failed loading tls key pair: %w", err)
@@ -163,8 +178,6 @@ func (cr *CertificateReloader) GetCertificate(*tls.ClientHelloInfo) (*tls.Certif
 
 	cr.certificate = &pair
 	cr.lastUpdatedAt = stat.ModTime()
-	}
-
 	return cr.certificate, nil
 }
 

utils/logger.go

@@ -8,14 +8,25 @@ import (
 	"gopkg.in/natefinch/lumberjack.v2"
 )
 
-var consoleWriter = zerolog.ConsoleWriter{Out: os.Stdout, TimeFormat: time.RFC3339}
-var fileWriter = &lumberjack.Logger{
-	Filename:   "/var/log/local-ip.sh.log",
-	MaxBackups: 3,
-	MaxSize:    1,    // megabytes
-	MaxAge:     1,    // days
-	Compress:   true, // disabled by default
-}
-var multi = zerolog.MultiLevelWriter(consoleWriter, fileWriter)
+const DefaultLogFile = "/var/log/local-ip.sh.log"
 
-var Logger = zerolog.New(multi).With().Timestamp().Logger()
+var consoleWriter = zerolog.ConsoleWriter{Out: os.Stdout, TimeFormat: time.RFC3339}
+
+var Logger = zerolog.New(consoleWriter).With().Timestamp().Logger()
+
+func InitLogger(logFile string) {
+	if logFile == "" {
+		logFile = DefaultLogFile
+	}
+
+	fileWriter := &lumberjack.Logger{
+		Filename:   logFile,
+		MaxBackups: 3,
+		MaxSize:    1,
+		MaxAge:     1,
+		Compress:   true,
+	}
+
+	multi := zerolog.MultiLevelWriter(consoleWriter, fileWriter)
+	Logger = zerolog.New(multi).With().Timestamp().Logger()
+}

xip/records.go

@@ -15,8 +15,8 @@ type hardcodedRecord struct {
 	SRV   *dns.SRV
 }
 
-var hardcodedRecords = map[string]hardcodedRecord{
-	// additional records I set up to host emails, feel free to change or remove them for your own needs
+func initialRecords() map[string]hardcodedRecord {
+	return map[string]hardcodedRecord{
 		"local-ip.sh.": {
 			TXT: []string{"v=spf1 include:capsulecorp.dev ~all"},
 			MX: []*dns.MX{
@@ -50,3 +50,4 @@ var hardcodedRecords = map[string]hardcodedRecord{
 			},
 		},
 	}
+}

xip/xip.go

@@ -6,6 +6,7 @@ import (
 	"os"
 	"regexp"
 	"strings"
+	"sync"
 	"time"
 
 	"github.com/miekg/dns"
@@ -15,6 +16,48 @@ import (
 type Xip struct {
 	server      dns.Server
 	nameServers []string
+	domain      string
+	email       string
+	dnsPort     uint
+	recordsMu   sync.RWMutex
+	records     map[string]hardcodedRecord
+}
+
+type Option func(*Xip)
+
+func WithDomain(domain string) Option {
+	return func(x *Xip) {
+		x.domain = domain
+	}
+}
+
+func WithEmail(email string) Option {
+	return func(x *Xip) {
+		x.email = email
+	}
+}
+
+func WithDnsPort(port uint) Option {
+	return func(x *Xip) {
+		x.dnsPort = port
+	}
+}
+
+func WithNameServers(nameServers []string) Option {
+	return func(x *Xip) {
+		x.recordsMu.Lock()
+		defer x.recordsMu.Unlock()
+		for i, ns := range nameServers {
+			name := fmt.Sprintf("ns%d.%s.", i+1, x.domain)
+			ip := net.ParseIP(ns)
+
+			entry := x.records[name]
+			entry.A = append(entry.A, ip)
+			x.records[name] = entry
+
+			x.nameServers = append(x.nameServers, name)
+		}
+	}
 }
 
 var (
@@ -26,38 +69,43 @@ var (
 
 func (xip *Xip) SetTXTRecord(fqdn string, value string) {
 	utils.Logger.Trace().Str("fqdn", fqdn).Str("value", value).Msg("Trying to set TXT record")
-	config := utils.GetConfig()
-	if fqdn != fmt.Sprintf("_acme-challenge.%s.", config.Domain) {
+	if fqdn != fmt.Sprintf("_acme-challenge.%s.", xip.domain) {
 		utils.Logger.Trace().Str("fqdn", fqdn).Msg("Not allowed, abort setting TXT record")
 		return
 	}
 
-	if rootRecords, ok := hardcodedRecords[fqdn]; ok {
+	xip.recordsMu.Lock()
+	defer xip.recordsMu.Unlock()
+	if rootRecords, ok := xip.records[fqdn]; ok {
 		rootRecords.TXT = []string{value}
-		hardcodedRecords[fmt.Sprintf("_acme-challenge.%s.", config.Domain)] = rootRecords
+		xip.records[fmt.Sprintf("_acme-challenge.%s.", xip.domain)] = rootRecords
 	}
 }
 
 func (xip *Xip) UnsetTXTRecord(fqdn string) {
 	utils.Logger.Trace().Str("fqdn", fqdn).Msg("Trying to unset TXT record")
-	config := utils.GetConfig()
-	if fqdn != fmt.Sprintf("_acme-challenge.%s.", config.Domain) {
+	if fqdn != fmt.Sprintf("_acme-challenge.%s.", xip.domain) {
 		utils.Logger.Trace().Str("fqdn", fqdn).Msg("Not allowed, abort unsetting TXT record")
 		return
 	}
 
-	if rootRecords, ok := hardcodedRecords[fqdn]; ok {
+	xip.recordsMu.Lock()
+	defer xip.recordsMu.Unlock()
+	if rootRecords, ok := xip.records[fqdn]; ok {
 		rootRecords.TXT = []string{}
-		hardcodedRecords[fmt.Sprintf("_acme-challenge.%s.", config.Domain)] = rootRecords
+		xip.records[fmt.Sprintf("_acme-challenge.%s.", xip.domain)] = rootRecords
 	}
 }
 
 func (xip *Xip) fqdnToA(fqdn string) []*dns.A {
 	normalizedFqdn := strings.ToLower(fqdn)
-	if hardcodedRecords[normalizedFqdn].A != nil {
+	xip.recordsMu.RLock()
+	records := xip.records[normalizedFqdn].A
+	xip.recordsMu.RUnlock()
+	if records != nil {
 		var aRecords []*dns.A
 
-		for _, record := range hardcodedRecords[normalizedFqdn].A {
+		for _, record := range records {
 			aRecords = append(aRecords, &dns.A{
 				Hdr: dns.RR_Header{
 					Ttl:    uint32((time.Minute * 5).Seconds()),
@@ -118,12 +166,15 @@ func (xip *Xip) handleA(question dns.Question, message *dns.Msg) {
 func (xip *Xip) handleAAAA(question dns.Question, message *dns.Msg) {
 	fqdn := question.Name
 	normalizedFqdn := strings.ToLower(fqdn)
-	if hardcodedRecords[normalizedFqdn].AAAA == nil {
+	xip.recordsMu.RLock()
+	records := xip.records[normalizedFqdn].AAAA
+	xip.recordsMu.RUnlock()
+	if records == nil {
 		xip.answerWithAuthority(question, message)
 		return
 	}
 
-	for _, record := range hardcodedRecords[normalizedFqdn].AAAA {
+	for _, record := range records {
 		message.Answer = append(message.Answer, &dns.AAAA{
 			Hdr: dns.RR_Header{
 				Ttl:    uint32((time.Minute * 5).Seconds()),
@@ -173,12 +224,15 @@ func chunkBy(str string, chunkSize int) (chunks []string) {
 func (xip *Xip) handleTXT(question dns.Question, message *dns.Msg) {
 	fqdn := question.Name
 	normalizedFqdn := strings.ToLower(fqdn)
-	if hardcodedRecords[normalizedFqdn].TXT == nil {
+	xip.recordsMu.RLock()
+	records := xip.records[normalizedFqdn].TXT
+	xip.recordsMu.RUnlock()
+	if records == nil {
 		xip.answerWithAuthority(question, message)
 		return
 	}
 
-	for _, record := range hardcodedRecords[normalizedFqdn].TXT {
+	for _, record := range records {
 		message.Answer = append(message.Answer, &dns.TXT{
 			Hdr: dns.RR_Header{
 				Ttl:    uint32((time.Minute * 5).Seconds()),
@@ -194,12 +248,15 @@ func (xip *Xip) handleTXT(question dns.Question, message *dns.Msg) {
 func (xip *Xip) handleMX(question dns.Question, message *dns.Msg) {
 	fqdn := question.Name
 	normalizedFqdn := strings.ToLower(fqdn)
-	if hardcodedRecords[normalizedFqdn].MX == nil {
+	xip.recordsMu.RLock()
+	records := xip.records[normalizedFqdn].MX
+	xip.recordsMu.RUnlock()
+	if records == nil {
 		xip.answerWithAuthority(question, message)
 		return
 	}
 
-	for _, record := range hardcodedRecords[normalizedFqdn].MX {
+	for _, record := range records {
 		message.Answer = append(message.Answer, &dns.MX{
 			Hdr: dns.RR_Header{
 				Ttl:    uint32((time.Minute * 5).Seconds()),
@@ -216,12 +273,15 @@ func (xip *Xip) handleMX(question dns.Question, message *dns.Msg) {
 func (xip *Xip) handleCNAME(question dns.Question, message *dns.Msg) {
 	fqdn := question.Name
 	normalizedFqdn := strings.ToLower(fqdn)
-	if hardcodedRecords[normalizedFqdn].CNAME == nil {
+	xip.recordsMu.RLock()
+	records := xip.records[normalizedFqdn].CNAME
+	xip.recordsMu.RUnlock()
+	if records == nil {
 		xip.answerWithAuthority(question, message)
 		return
 	}
 
-	for _, record := range hardcodedRecords[normalizedFqdn].CNAME {
+	for _, record := range records {
 		message.Answer = append(message.Answer, &dns.CNAME{
 			Hdr: dns.RR_Header{
 				Ttl:    uint32((time.Minute * 5).Seconds()),
@@ -237,7 +297,10 @@ func (xip *Xip) handleCNAME(question dns.Question, message *dns.Msg) {
 func (xip *Xip) handleSRV(question dns.Question, message *dns.Msg) {
 	fqdn := question.Name
 	normalizedFqdn := strings.ToLower(fqdn)
-	if hardcodedRecords[normalizedFqdn].SRV == nil {
+	xip.recordsMu.RLock()
+	record := xip.records[normalizedFqdn].SRV
+	xip.recordsMu.RUnlock()
+	if record == nil {
 		xip.answerWithAuthority(question, message)
 		return
 	}
@@ -249,10 +312,10 @@ func (xip *Xip) handleSRV(question dns.Question, message *dns.Msg) {
 			Rrtype: dns.TypeSRV,
 			Class:  dns.ClassINET,
 		},
-		Priority: hardcodedRecords[normalizedFqdn].SRV.Priority,
-		Weight:   hardcodedRecords[normalizedFqdn].SRV.Weight,
-		Port:     hardcodedRecords[normalizedFqdn].SRV.Port,
-		Target:   hardcodedRecords[normalizedFqdn].SRV.Target,
+		Priority: record.Priority,
+		Weight:   record.Weight,
+		Port:     record.Port,
+		Target:   record.Target,
 	})
 }
 
@@ -268,7 +331,6 @@ func emailToRname(email string) string {
 }
 
 func (xip *Xip) soaRecord(question dns.Question) *dns.SOA {
-	config := utils.GetConfig()
 	soa := new(dns.SOA)
 	soa.Hdr = dns.RR_Header{
 		Name:     question.Name,
@@ -278,7 +340,7 @@ func (xip *Xip) soaRecord(question dns.Question) *dns.SOA {
 		Rdlength: 0,
 	}
 	soa.Ns = xip.nameServers[0]
-	soa.Mbox = emailToRname(config.Email)
+	soa.Mbox = emailToRname(xip.email)
 	soa.Serial = 2024072600
 	soa.Refresh = uint32((time.Minute * 15).Seconds())
 	soa.Retry = uint32((time.Minute * 15).Seconds())
@@ -363,7 +425,7 @@ func (xip *Xip) StartServer() {
 	err := xip.server.ListenAndServe()
 	defer xip.server.Shutdown()
 	if err != nil {
-		utils.Logger.Fatal().Err(err).Msg("Failed to start DNS server")
+		utils.Logger.Error().Err(err).Msg("Failed to start DNS server")
 		if strings.Contains(err.Error(), "fly-global-services: no such host") {
 			// we're not running on fly, bind to 0.0.0.0 instead
 			port := strings.Split(xip.server.Addr, ":")[1]
@@ -381,40 +443,52 @@ func (xip *Xip) StartServer() {
 	utils.Logger.Info().Str("dns_address", xip.server.Addr).Msg("Starting up DNS server")
 }
 
-func (xip *Xip) initHardcodedRecords() {
-	config := utils.GetConfig()
+func (xip *Xip) initNameServers(nameServers []string) {
 	rootDomainARecords := []net.IP{}
 
-	for i, ns := range config.NameServers {
-		name := fmt.Sprintf("ns%d.%s.", i+1, config.Domain)
+	xip.recordsMu.Lock()
+	defer xip.recordsMu.Unlock()
+
+	for i, ns := range nameServers {
+		name := fmt.Sprintf("ns%d.%s.", i+1, xip.domain)
 		ip := net.ParseIP(ns)
 
 		rootDomainARecords = append(rootDomainARecords, ip)
-		entry := hardcodedRecords[name]
-		entry.A = append(hardcodedRecords[name].A, ip)
-		hardcodedRecords[name] = entry
+		entry := xip.records[name]
+		entry.A = append(xip.records[name].A, ip)
+		xip.records[name] = entry
 
 		xip.nameServers = append(xip.nameServers, name)
 	}
 
-	hardcodedRecords[fmt.Sprintf("%s.", config.Domain)] = hardcodedRecord{A: rootDomainARecords}
+	xip.records[fmt.Sprintf("%s.", xip.domain)] = hardcodedRecord{A: rootDomainARecords}
 
-	// will be filled in later when requesting certificates
-	hardcodedRecords[fmt.Sprintf("_acme-challenge.%s.", config.Domain)] = hardcodedRecord{TXT: []string{}}
+	xip.records[fmt.Sprintf("_acme-challenge.%s.", xip.domain)] = hardcodedRecord{TXT: []string{}}
 }
 
-func NewXip() (xip *Xip) {
+func NewXip(opts ...Option) (xip *Xip) {
 	config := utils.GetConfig()
-	xip = &Xip{}
+	xip = &Xip{
+		domain:  config.Domain,
+		email:   config.Email,
+		dnsPort: config.DnsPort,
+		records: initialRecords(),
+	}
 
-	xip.initHardcodedRecords()
+	for _, opt := range opts {
+		opt(xip)
+	}
+
+	if len(xip.nameServers) == 0 {
+		xip.initNameServers(config.NameServers)
+	}
 
 	xip.server = dns.Server{
-		Addr: fmt.Sprintf(":%d", config.DnsPort),
+		Addr: fmt.Sprintf(":%d", xip.dnsPort),
 		Net:  "udp",
 	}
 
-	zone := fmt.Sprintf("%s.", config.Domain)
+	zone := fmt.Sprintf("%s.", xip.domain)
 	dns.HandleFunc(zone, xip.handleDnsRequest)
 
 	return xip

xip/xip_test.go

@@ -5,13 +5,14 @@ import (
 	"os/exec"
 	"strings"
 	"testing"
-
-	"github.com/spf13/viper"
 )
 
 func TestResolveDashUnit(t *testing.T) {
-	// viper.Set("dns-port", 9053)
-	xip := NewXip()
+	xip := NewXip(
+		WithDomain("local-ip.sh"),
+		WithDnsPort(9053),
+		WithNameServers([]string{"1.2.3.4", "5.6.7.8"}),
+	)
 
 	A := xip.fqdnToA("192-168-1-29.local-ip.sh")
 	expected := "192.168.1.29"
@@ -41,20 +42,26 @@ func TestResolveDashUnit(t *testing.T) {
 }
 
 func TestConstructor(t *testing.T) {
-	viper.Set("dns-port", 9053)
-	xip := NewXip()
+	xip := NewXip(
+		WithDomain("local-ip.sh"),
+		WithDnsPort(9053),
+		WithNameServers([]string{"1.2.3.4", "5.6.7.8"}),
+	)
 
-	if xip.nameServers[0] != "ns1.local-ip.sh" {
-		t.Error("")
+	if xip.nameServers[0] != "ns1.local-ip.sh." {
+		t.Errorf("expected ns1.local-ip.sh., got %s", xip.nameServers[0])
 	}
-	if xip.nameServers[1] != "ns2.local-ip.sh" {
-		t.Error("")
+	if xip.nameServers[1] != "ns2.local-ip.sh." {
+		t.Errorf("expected ns2.local-ip.sh., got %s", xip.nameServers[1])
 	}
 }
 
 func TestResolveDashE2E(t *testing.T) {
-	viper.Set("dns-port", 9053)
-	xip := NewXip()
+	xip := NewXip(
+		WithDomain("local-ip.sh"),
+		WithDnsPort(9053),
+		WithNameServers([]string{"1.2.3.4", "5.6.7.8"}),
+	)
 	go xip.StartServer()
 
 	cmd := exec.Command("dig", "@localhost", "-p", "9053", "192-168-1-29.local-ip.sh", "+short")
@@ -70,25 +77,55 @@ func TestResolveDashE2E(t *testing.T) {
 
 func BenchmarkResolveDashBasic(b *testing.B) {
 	b.Skip()
-	// var semaphore = make(chan int, 40)
-	// var done = make(chan bool, 1)
 
 	for i := 0; i < b.N; i++ {
 		port := 9053 + i
-		viper.Set("dns-port", port)
-		xip := NewXip()
+		xip := NewXip(
+			WithDomain("local-ip.sh"),
+			WithDnsPort(uint(port)),
+			WithNameServers([]string{"1.2.3.4", "5.6.7.8"}),
+		)
 		go xip.StartServer()
 
-		// semaphore <- 1
-		// go func() {
 		cmd := exec.Command("dig", "@localhost", "-p", fmt.Sprint(port), "192-168-1-29.local-ip.sh", "+short")
 		cmd.Run()
+	}
+}
 
-		// <-semaphore
-		// if i == b.N {
-		// done <- true
-		// }
-		// }()
+func TestInstanceIsolation(t *testing.T) {
+	xip1 := NewXip(
+		WithDomain("one.test"),
+		WithDnsPort(0),
+		WithNameServers([]string{"1.1.1.1"}),
+	)
+	xip2 := NewXip(
+		WithDomain("two.test"),
+		WithDnsPort(0),
+		WithNameServers([]string{"2.2.2.2"}),
+	)
+
+	if xip1.records == nil || xip2.records == nil {
+		t.Fatal("records not initialized")
+	}
+
+	if len(xip1.nameServers) != 1 || xip1.nameServers[0] != "ns1.one.test." {
+		t.Errorf("xip1 nameservers incorrect: %v", xip1.nameServers)
+	}
+	if len(xip2.nameServers) != 1 || xip2.nameServers[0] != "ns1.two.test." {
+		t.Errorf("xip2 nameservers incorrect: %v", xip2.nameServers)
+	}
+
+	if _, ok := xip1.records["ns1.one.test."]; !ok {
+		t.Error("xip1 missing ns1.one.test. record")
+	}
+	if _, ok := xip2.records["ns1.two.test."]; !ok {
+		t.Error("xip2 missing ns1.two.test. record")
+	}
+
+	if _, ok := xip1.records["ns1.two.test."]; ok {
+		t.Error("xip1 should not have xip2's records")
+	}
+	if _, ok := xip2.records["ns1.one.test."]; ok {
+		t.Error("xip2 should not have xip1's records")
 	}
-	// <-done
 }