feat(htpasswd): use dedicated fsnotify reloader for htpasswd file

- rewrite htpasswd watcher not to store context
- improve logging

Signed-off-by: Vladimir Ermakov <[email protected]>

pkg/api/controller.go

@@ -51,6 +51,7 @@ type Controller struct {
 	RelyingParties  map[string]rp.RelyingParty
 	CookieStore     *CookieStore
 	HTPasswd        *HTPasswd
+	HTPasswdWatcher *HTPasswdWatcher
 	LDAPClient      *LDAPClient
 	taskScheduler   *scheduler.Scheduler
 	// runtime params
@@ -99,9 +100,17 @@ func NewController(appConfig *config.Config) *Controller {
 			Str("clusterMemberIndex", strconv.Itoa(memberSocketIdx)).Logger()
 	}
 
+	htp := NewHTPasswd(logger)
+
+	htw, err := NewHTPasswdWatcher(htp, "")
+	if err != nil {
+		logger.Panic().Err(err).Msg("failed to create htpasswd watcher")
+	}
+
 	controller.Config = appConfig
 	controller.Log = logger
-	controller.HTPasswd = NewHTPasswd(logger)
+	controller.HTPasswd = htp
+	controller.HTPasswdWatcher = htw
 
 	if appConfig.Log.Audit != "" {
 		audit := log.NewAuditLogger(appConfig.Log.Level, appConfig.Log.Audit)
@@ -285,6 +294,13 @@ func (c *Controller) Init() error {
 
 	c.InitCVEInfo()
 
+	if c.Config.IsHtpasswdAuthEnabled() {
+		err := c.HTPasswdWatcher.ChangeFile(c.Config.HTTP.Auth.HTPasswd.Path)
+		if err != nil {
+			return err
+		}
+	}
+
 	return nil
 }
 
@@ -367,10 +383,9 @@ func (c *Controller) LoadNewConfig(newConfig *config.Config) {
 		c.Config.HTTP.Auth.HTPasswd = newConfig.HTTP.Auth.HTPasswd
 		c.Config.HTTP.Auth.LDAP = newConfig.HTTP.Auth.LDAP
 
-		if c.Config.HTTP.Auth.HTPasswd.Path == "" {
-			c.HTPasswd.Clear()
-		} else {
-			_ = c.HTPasswd.Reload(c.Config.HTTP.Auth.HTPasswd.Path)
+		err := c.HTPasswdWatcher.ChangeFile(c.Config.HTTP.Auth.HTPasswd.Path)
+		if err != nil {
+			c.Log.Error().Err(err).Msg("failed to change watched htpasswd file")
 		}
 
 		if c.LDAPClient != nil {
@@ -379,6 +394,8 @@ func (c *Controller) LoadNewConfig(newConfig *config.Config) {
 			c.LDAPClient.BindPassword = newConfig.HTTP.Auth.LDAP.BindPassword()
 			c.LDAPClient.lock.Unlock()
 		}
+	} else {
+		_ = c.HTPasswdWatcher.ChangeFile("")
 	}
 
 	// reload periodical gc config

pkg/api/controller_test.go

@@ -3142,7 +3142,7 @@ func TestBasicAuthWithReloadedCredentials(t *testing.T) {
 		ctlr := api.NewController(conf)
 		ctlrManager := test.NewControllerManager(ctlr)
 
-		hotReloader, err := server.NewHotReloader(ctlr, configPath, "", ldapConfigPath)
+		hotReloader, err := server.NewHotReloader(ctlr, configPath, ldapConfigPath)
 		So(err, ShouldBeNil)
 
 		hotReloader.Start()

pkg/api/htpasswd.go

@@ -2,15 +2,23 @@ package api
 
 import (
 	"bufio"
+	"context"
+	"errors"
 	"os"
+	"os/signal"
 	"strings"
 	"sync"
+	"syscall"
 
+	"github.com/fsnotify/fsnotify"
 	"golang.org/x/crypto/bcrypt"
 
 	"zotregistry.dev/zot/pkg/log"
 )
 
+// HTPasswd user auth store
+//
+// Currently supports only bcrypt hashes.
 type HTPasswd struct {
 	mu      sync.RWMutex
 	credMap map[string]string
@@ -29,7 +37,7 @@ func (s *HTPasswd) Reload(filePath string) error {
 
 	credsFile, err := os.Open(filePath)
 	if err != nil {
-		s.log.Error().Err(err).Str("credsFile", filePath).Msg("failed to reload htpasswd")
+		s.log.Error().Err(err).Str("htpasswd-file", filePath).Msg("failed to reload htpasswd")
 
 		return err
 	}
@@ -38,15 +46,16 @@ func (s *HTPasswd) Reload(filePath string) error {
 	scanner := bufio.NewScanner(credsFile)
 
 	for scanner.Scan() {
-		line := scanner.Text()
-		if strings.Contains(line, ":") {
-			tokens := strings.Split(scanner.Text(), ":")
-			credMap[tokens[0]] = tokens[1]
+		user, hash, ok := strings.Cut(scanner.Text(), ":")
+		if ok {
+			credMap[user] = hash
 		}
 	}
 
 	if len(credMap) == 0 {
-		s.log.Warn().Str("credsFile", filePath).Msg("loaded htpasswd file appears to have zero users")
+		s.log.Warn().Str("htpasswd-file", filePath).Msg("loaded htpasswd file appears to have zero users")
+	} else {
+		s.log.Info().Str("htpasswd-file", filePath).Int("users", len(credMap)).Msg("loaded htpasswd file")
 	}
 
 	s.mu.Lock()
@@ -81,5 +90,109 @@ func (s *HTPasswd) Authenticate(username, passphrase string) (ok, present bool)
 	err := bcrypt.CompareHashAndPassword([]byte(passphraseHash), []byte(passphrase))
 	ok = err == nil
 
+	if err != nil && !errors.Is(err, bcrypt.ErrMismatchedHashAndPassword) {
+		// Log that user's hash has unsupported format. Better than silently return 401.
+		s.log.Warn().Err(err).Str("username", username).Msg("htpasswd bcrypt compare failed")
+	}
+
 	return
 }
+
+// HTPasswdWatcher helper which triggers htpasswd reload on file change event.
+//
+// Cannot be restarted.
+type HTPasswdWatcher struct {
+	htp      *HTPasswd
+	filePath string
+	watcher  *fsnotify.Watcher
+	cancel   context.CancelFunc
+	log      log.Logger
+}
+
+// NewHTPasswdWatcher create and start watcher.
+func NewHTPasswdWatcher(htp *HTPasswd, filePath string) (*HTPasswdWatcher, error) {
+	watcher, err := fsnotify.NewWatcher()
+	if err != nil {
+		return nil, err
+	}
+
+	if filePath != "" {
+		err = watcher.Add(filePath)
+		if err != nil {
+			return nil, errors.Join(err, watcher.Close())
+		}
+	}
+
+	// background event processor job context
+	ctx, cancel := signal.NotifyContext(context.Background(), syscall.SIGTERM, syscall.SIGINT)
+
+	ret := &HTPasswdWatcher{
+		htp:      htp,
+		filePath: filePath,
+		watcher:  watcher,
+		cancel:   cancel,
+		log:      htp.log,
+	}
+
+	go func() {
+		defer ret.watcher.Close() //nolint: errcheck
+
+		for {
+			select {
+			case ev := <-ret.watcher.Events:
+				if ev.Op != fsnotify.Write {
+					continue
+				}
+
+				ret.log.Info().Str("htpasswd-file", ret.filePath).Msg("htpasswd file changed, trying to reload config")
+
+				err := ret.htp.Reload(ret.filePath)
+				if err != nil {
+					ret.log.Warn().Err(err).Str("htpasswd-file", ret.filePath).Msg("failed to reload file")
+				}
+
+			case err := <-ret.watcher.Errors:
+				ret.log.Error().Err(err).Str("htpasswd-file", ret.filePath).Msg("failed to fsnotfy, got error while watching file")
+
+			case <-ctx.Done():
+				ret.log.Debug().Msg("htpasswd watcher terminating...")
+
+				return
+			}
+		}
+	}()
+
+	return ret, nil
+}
+
+// ChangeFile changes monitored file. Empty string clears store.
+func (s *HTPasswdWatcher) ChangeFile(filePath string) error {
+	if s.filePath != "" {
+		err := s.watcher.Remove(s.filePath)
+		if err != nil {
+			return err
+		}
+	}
+
+	if filePath == "" {
+		s.filePath = filePath
+		s.htp.Clear()
+
+		return nil
+	}
+
+	err := s.watcher.Add(filePath)
+	if err != nil {
+		return err
+	}
+
+	s.filePath = filePath
+
+	return s.htp.Reload(filePath)
+}
+
+func (s *HTPasswdWatcher) Close() error {
+	s.cancel()
+
+	return nil
+}

pkg/cli/server/config_reloader.go

@@ -16,12 +16,11 @@ import (
 type HotReloader struct {
 	watcher             *fsnotify.Watcher
 	configPath          string
-	htpasswdPath        string
 	ldapCredentialsPath string
 	ctlr                *api.Controller
 }
 
-func NewHotReloader(ctlr *api.Controller, filePath, htpasswdPath, ldapCredentialsPath string) (*HotReloader, error) {
+func NewHotReloader(ctlr *api.Controller, filePath, ldapCredentialsPath string) (*HotReloader, error) {
 	// creates a new file watcher
 	watcher, err := fsnotify.NewWatcher()
 	if err != nil {
@@ -31,7 +30,6 @@ func NewHotReloader(ctlr *api.Controller, filePath, htpasswdPath, ldapCredential
 	hotReloader := &HotReloader{
 		watcher:             watcher,
 		configPath:          filePath,
-		htpasswdPath:        htpasswdPath,
 		ldapCredentialsPath: ldapCredentialsPath,
 		ctlr:                ctlr,
 	}
@@ -85,20 +83,6 @@ func (hr *HotReloader) Start() {
 							continue
 						}
 
-						if hr.ctlr.Config.HTTP.Auth != nil &&
-							hr.ctlr.Config.HTTP.Auth.HTPasswd.Path != newConfig.HTTP.Auth.HTPasswd.Path {
-							err = hr.watcher.Remove(hr.ctlr.Config.HTTP.Auth.HTPasswd.Path)
-							if err != nil && !errors.Is(err, fsnotify.ErrNonExistentWatch) {
-								log.Error().Err(err).Msg("failed to remove old watch for the htpasswd file")
-							}
-
-							err = hr.watcher.Add(newConfig.HTTP.Auth.HTPasswd.Path)
-							if err != nil {
-								log.Panic().Err(err).Str("htpasswd-file", newConfig.HTTP.Auth.HTPasswd.Path).
-									Msg("failed to watch htpasswd file")
-							}
-						}
-
 						if hr.ctlr.Config.HTTP.Auth != nil && hr.ctlr.Config.HTTP.Auth.LDAP != nil &&
 							hr.ctlr.Config.HTTP.Auth.LDAP.CredentialsFile != newConfig.HTTP.Auth.LDAP.CredentialsFile {
 							err = hr.watcher.Remove(hr.ctlr.Config.HTTP.Auth.LDAP.CredentialsFile)
@@ -133,13 +117,6 @@ func (hr *HotReloader) Start() {
 			log.Panic().Err(err).Str("config", hr.configPath).Msg("failed to add config file to fsnotity watcher")
 		}
 
-		if hr.htpasswdPath != "" {
-			if err := hr.watcher.Add(hr.htpasswdPath); err != nil {
-				log.Panic().Err(err).Str("htpasswd-file", hr.htpasswdPath).
-					Msg("failed to add htpasswd to fsnotity watcher")
-			}
-		}
-
 		if hr.ldapCredentialsPath != "" {
 			if err := hr.watcher.Add(hr.ldapCredentialsPath); err != nil {
 				log.Panic().Err(err).Str("ldap-credentials", hr.ldapCredentialsPath).

pkg/cli/server/root.go

@@ -57,18 +57,13 @@ func newServeCmd(conf *config.Config) *cobra.Command {
 
 			ctlr := api.NewController(conf)
 
-			htpasswdPath := ""
 			ldapCredentials := ""
 
-			if conf.HTTP.Auth != nil {
-				htpasswdPath = conf.HTTP.Auth.HTPasswd.Path
-			}
-
 			if conf.HTTP.Auth != nil && conf.HTTP.Auth.LDAP != nil {
 				ldapCredentials = conf.HTTP.Auth.LDAP.CredentialsFile
 			}
 			// config reloader
-			hotReloader, err := NewHotReloader(ctlr, args[0], htpasswdPath, ldapCredentials)
+			hotReloader, err := NewHotReloader(ctlr, args[0], ldapCredentials)
 			if err != nil {
 				ctlr.Log.Error().Err(err).Msg("failed to create a new hot reloader")
 

pkg/extensions/sync/sync_test.go

@@ -2069,7 +2069,7 @@ func TestConfigReloader(t *testing.T) {
 			_, err = cfgfile.WriteString(content)
 			So(err, ShouldBeNil)
 
-			hotReloader, err := cli.NewHotReloader(dctlr, cfgfile.Name(), "", "")
+			hotReloader, err := cli.NewHotReloader(dctlr, cfgfile.Name(), "")
 			So(err, ShouldBeNil)
 
 			hotReloader.Start()
@@ -2219,7 +2219,7 @@ func TestConfigReloader(t *testing.T) {
 			_, err = cfgfile.WriteString(content)
 			So(err, ShouldBeNil)
 
-			hotReloader, err := cli.NewHotReloader(dctlr, cfgfile.Name(), "", "")
+			hotReloader, err := cli.NewHotReloader(dctlr, cfgfile.Name(), "")
 			So(err, ShouldBeNil)
 
 			hotReloader.Start()