From 667d557c1e4a271374364cba8eb5fe8ae4dd4864 Mon Sep 17 00:00:00 2001 From: Alexandru Ciobanu Date: Fri, 24 Jun 2022 13:30:51 +0300 Subject: [PATCH 1/9] dynamically update the htpasswdMap based on the changes made to the htpasswd file --- oauthproxy.go | 2 +- pkg/authentication/basic/htpasswd.go | 89 +++++++++++++++++++++------- pkg/watcher/watcher.go | 89 ++++++++++++++++++++++++++++ validator.go | 3 +- watcher.go | 81 ------------------------- watcher_unsupported.go | 11 ---- 6 files changed, 159 insertions(+), 116 deletions(-) create mode 100644 pkg/watcher/watcher.go delete mode 100644 watcher.go delete mode 100644 watcher_unsupported.go diff --git a/oauthproxy.go b/oauthproxy.go index cd2a331153..02ea2d7535 100644 --- a/oauthproxy.go +++ b/oauthproxy.go @@ -113,7 +113,7 @@ func NewOAuthProxy(opts *options.Options, validator func(string) bool) (*OAuthPr var err error basicAuthValidator, err = basic.NewHTPasswdValidator(opts.HtpasswdFile) if err != nil { - return nil, fmt.Errorf("could not load htpasswdfile: %v", err) + return nil, fmt.Errorf("could not load htpasswd file: %v", err) } } diff --git a/pkg/authentication/basic/htpasswd.go b/pkg/authentication/basic/htpasswd.go index 47a1dd3f14..f16097c9d1 100644 --- a/pkg/authentication/basic/htpasswd.go +++ b/pkg/authentication/basic/htpasswd.go @@ -8,8 +8,11 @@ import ( "fmt" "io" "os" + "strings" + "sync" "github.com/oauth2-proxy/oauth2-proxy/v7/pkg/logger" + "github.com/oauth2-proxy/oauth2-proxy/v7/pkg/watcher" "golang.org/x/crypto/bcrypt" ) @@ -17,6 +20,7 @@ import ( // Passwords must be generated with -B for bcrypt or -s for SHA1. type htpasswdMap struct { users map[string]interface{} + rwm sync.RWMutex } // bcryptPass is used to identify bcrypt passwords in the @@ -30,10 +34,29 @@ type sha1Pass string // NewHTPasswdValidator constructs an httpasswd based validator from the file // at the path given. func NewHTPasswdValidator(path string) (Validator, error) { + h := &htpasswdMap{users: make(map[string]interface{})} + + err := h.loadHTPasswdFile(path) + if err != nil { + return nil, err + } + + watcher.WatchFileForUpdates(path, nil, func() { + err := h.loadHTPasswdFile(path) + if err != nil { + logger.Error(err) + } + }) + + return h, nil +} + +// loadHTPasswdFile loads htpasswd entries from an io.Reader (an opened file) into a htpasswdMap. +func (h *htpasswdMap) loadHTPasswdFile(filename string) error { // We allow HTPasswd location via config options - r, err := os.Open(path) // #nosec G304 + r, err := os.Open(filename) // #nosec G304 if err != nil { - return nil, fmt.Errorf("could not open htpasswd file: %v", err) + return fmt.Errorf("could not open htpasswd file: %v", err) } defer func(c io.Closer) { cerr := c.Close() @@ -41,45 +64,67 @@ func NewHTPasswdValidator(path string) (Validator, error) { logger.Fatalf("error closing the htpasswd file: %v", cerr) } }(r) - return newHtpasswd(r) -} -// newHtpasswd consctructs an htpasswd from an io.Reader (an opened file). -func newHtpasswd(file io.Reader) (*htpasswdMap, error) { - csvReader := csv.NewReader(file) + csvReader := csv.NewReader(r) csvReader.Comma = ':' csvReader.Comment = '#' csvReader.TrimLeadingSpace = true records, err := csvReader.ReadAll() if err != nil { - return nil, fmt.Errorf("could not read htpasswd file: %v", err) + logger.Fatalf("could not read htpasswd file: %v", err) + } + + updated, err := createHtpasswdMap(records) + if err != nil { + return fmt.Errorf("htpasswd entries error: %v", err) } - return createHtpasswdMap(records) + h.rwm.Lock() + h.users = updated.users + h.rwm.Unlock() + + return nil } // createHtasswdMap constructs an htpasswdMap from the given records func createHtpasswdMap(records [][]string) (*htpasswdMap, error) { h := &htpasswdMap{users: make(map[string]interface{})} + var invalidRecords, invalidEntries []string for _, record := range records { - user, realPassword := record[0], record[1] - shaPrefix := realPassword[:5] - if shaPrefix == "{SHA}" { - h.users[user] = sha1Pass(realPassword[5:]) - continue + // If a record is invalid or malformed don't panic with index out of range, + // return a formatted error. + lr := len(record) + switch { + case lr == 2: + user, realPassword := record[0], record[1] + if strings.HasPrefix(realPassword, "{SHA}") { + h.users[user] = sha1Pass(realPassword[5:]) + } else if strings.HasPrefix(realPassword, "$2b$") || + strings.HasPrefix(realPassword, "$2y$") || + strings.HasPrefix(realPassword, "$2x$") || + strings.HasPrefix(realPassword, "$2a$") { + h.users[user] = bcryptPass(realPassword) + } else { + invalidEntries = append(invalidEntries, user) + } + case lr == 1, lr > 2: + invalidRecords = append(invalidRecords, record[0]) } + } - bcryptPrefix := realPassword[:4] - if bcryptPrefix == "$2a$" || bcryptPrefix == "$2b$" || bcryptPrefix == "$2x$" || bcryptPrefix == "$2y$" { - h.users[user] = bcryptPass(realPassword) - continue - } + if len(invalidRecords) > 0 { + return h, fmt.Errorf("invalid htpasswd record(s) %+q", invalidRecords) + } - // Password is neither sha1 or bcrypt - // TODO(JoelSpeed): In the next breaking release, make this return an error. - logger.Errorf("Invalid htpasswd entry for %s. Must be a SHA or bcrypt entry.", user) + if len(invalidEntries) > 0 { + return h, fmt.Errorf("'%+q' user(s) could not be added: invalid password, must be a SHA or bcrypt entry", invalidEntries) } + + if len(h.users) == 0 { + logger.Fatal("could not construct htpasswdMap: htpasswd file doesn't contain a single valid user entry") + } + return h, nil } diff --git a/pkg/watcher/watcher.go b/pkg/watcher/watcher.go new file mode 100644 index 0000000000..845c484a1e --- /dev/null +++ b/pkg/watcher/watcher.go @@ -0,0 +1,89 @@ +package watcher + +import ( + "os" + "path/filepath" + "time" + + "github.com/fsnotify/fsnotify" + + "github.com/oauth2-proxy/oauth2-proxy/v7/pkg/logger" +) + +const writeOrCreateMask = fsnotify.Write | fsnotify.Create + +// WaitForReplacement waits for a file to exist on disk and then starts a watch +// for the file +func WaitForReplacement(filename string, op fsnotify.Op, + watcher *fsnotify.Watcher) { + const sleepInterval = 50 * time.Millisecond + + // Avoid a race when fsnofity.Remove is preceded by fsnotify.Chmod. + if op&fsnotify.Chmod != 0 { + time.Sleep(sleepInterval) + } + for { + if _, err := os.Stat(filename); err == nil { + if err := watcher.Add(filename); err == nil { + logger.Printf("watching resumed for '%s'", filename) + return + } + } + time.Sleep(sleepInterval) + } +} + +// WatchForUpdates performs an action every time a file on disk is updated +func WatchFileForUpdates(filename string, done <-chan bool, action func()) { + realFile, _ := filepath.EvalSymlinks(filename) + watcher, err := fsnotify.NewWatcher() + if err != nil { + logger.Fatalf("failed to create watcher for '%s': %s", filename, err) + } + + go func() { + defer func(w *fsnotify.Watcher) { + cerr := w.Close() + if cerr != nil { + logger.Fatalf("error closing watcher: %v", err) + } + }(watcher) + for { + select { + case <-done: + logger.Printf("shutting down watcher for: %s", filename) + return + case event, ok := <-watcher.Events: + // 'Events' channel is closed + if !ok { + logger.Errorf("error: cannot start the watcher, events channel is closed") + return + } + currentFile, _ := filepath.EvalSymlinks(filename) + // we only care about the config file with the following cases: + // 1 - if the file was modified or created + // 2 - if the real path to the file changed (eg: k8s ConfigMap/Secret replacement) + if (filepath.Clean(event.Name) == filename && + event.Op&writeOrCreateMask != 0) || + (currentFile != "" && currentFile != realFile) { + logger.Printf("reloading after event: %s", event) + realFile = currentFile + action() + } else if filepath.Clean(event.Name) == filename && + event.Op&fsnotify.Remove != 0 { + logger.Printf("watching interrupted on event: %s", event) + WaitForReplacement(filename, event.Op, watcher) + if err = watcher.Add(filename); err != nil { + logger.Fatalf("failed to add '%s' to watcher: %v", filename, err) + } + } + case err = <-watcher.Errors: + logger.Errorf("error watching '%s': %s", filename, err) + } + } + }() + if err = watcher.Add(filename); err != nil { + logger.Fatalf("failed to add '%s' to watcher: %v", filename, err) + } + logger.Printf("watching '%s' for updates", filename) +} diff --git a/validator.go b/validator.go index 7f5349a81d..587ef857fe 100644 --- a/validator.go +++ b/validator.go @@ -9,6 +9,7 @@ import ( "unsafe" "github.com/oauth2-proxy/oauth2-proxy/v7/pkg/logger" + "github.com/oauth2-proxy/oauth2-proxy/v7/pkg/watcher" ) // UserMap holds information from the authenticated emails file @@ -26,7 +27,7 @@ func NewUserMap(usersFile string, done <-chan bool, onUpdate func()) *UserMap { atomic.StorePointer(&um.m, unsafe.Pointer(&m)) // #nosec G103 if usersFile != "" { logger.Printf("using authenticated emails file %s", usersFile) - WatchForUpdates(usersFile, done, func() { + watcher.WatchFileForUpdates(usersFile, done, func() { um.LoadAuthenticatedEmailsFile() onUpdate() }) diff --git a/watcher.go b/watcher.go deleted file mode 100644 index 86c3e6532e..0000000000 --- a/watcher.go +++ /dev/null @@ -1,81 +0,0 @@ -//go:build go1.3 && !plan9 && !solaris -// +build go1.3,!plan9,!solaris - -package main - -import ( - "os" - "path/filepath" - "time" - - "github.com/fsnotify/fsnotify" - - "github.com/oauth2-proxy/oauth2-proxy/v7/pkg/logger" -) - -// WaitForReplacement waits for a file to exist on disk and then starts a watch -// for the file -func WaitForReplacement(filename string, op fsnotify.Op, - watcher *fsnotify.Watcher) { - const sleepInterval = 50 * time.Millisecond - - // Avoid a race when fsnofity.Remove is preceded by fsnotify.Chmod. - if op&fsnotify.Chmod != 0 { - time.Sleep(sleepInterval) - } - for { - if _, err := os.Stat(filename); err == nil { - if err := watcher.Add(filename); err == nil { - logger.Printf("watching resumed for %s", filename) - return - } - } - time.Sleep(sleepInterval) - } -} - -// WatchForUpdates performs an action every time a file on disk is updated -func WatchForUpdates(filename string, done <-chan bool, action func()) { - filename = filepath.Clean(filename) - watcher, err := fsnotify.NewWatcher() - if err != nil { - logger.Fatal("failed to create watcher for ", filename, ": ", err) - } - go func() { - defer func(w *fsnotify.Watcher) { - cerr := w.Close() - if cerr != nil { - logger.Fatalf("error closing watcher: %v", err) - } - }(watcher) - for { - select { - case <-done: - logger.Printf("Shutting down watcher for: %s", filename) - return - case event := <-watcher.Events: - // On Arch Linux, it appears Chmod events precede Remove events, - // which causes a race between action() and the coming Remove event. - // If the Remove wins, the action() (which calls - // UserMap.LoadAuthenticatedEmailsFile()) crashes when the file - // can't be opened. - if event.Op&(fsnotify.Remove|fsnotify.Rename|fsnotify.Chmod) != 0 { - logger.Printf("watching interrupted on event: %s", event) - err = watcher.Remove(filename) - if err != nil { - logger.Printf("error removing watcher on %s: %v", filename, err) - } - WaitForReplacement(filename, event.Op, watcher) - } - logger.Printf("reloading after event: %s", event) - action() - case err = <-watcher.Errors: - logger.Errorf("error watching %s: %s", filename, err) - } - } - }() - if err = watcher.Add(filename); err != nil { - logger.Fatal("failed to add ", filename, " to watcher: ", err) - } - logger.Printf("watching %s for updates", filename) -} diff --git a/watcher_unsupported.go b/watcher_unsupported.go deleted file mode 100644 index 0f9e5f67d6..0000000000 --- a/watcher_unsupported.go +++ /dev/null @@ -1,11 +0,0 @@ -//go:build !go1.3 || plan9 || solaris -// +build !go1.3 plan9 solaris - -package main - -import "github.com/oauth2-proxy/oauth2-proxy/v7/pkg/logger" - -func WatchForUpdates(filename string, done <-chan bool, action func()) { - logger.Errorf("file watching not implemented on this platform") - go func() { <-done }() -} From 0b3ef55a84b7f30724ce28b6062084010b5471ed Mon Sep 17 00:00:00 2001 From: Alexandru Ciobanu Date: Fri, 24 Jun 2022 13:38:46 +0300 Subject: [PATCH 2/9] added tests to validate that htpasswdMap is updated after the htpasswd file is changed --- pkg/authentication/basic/htpasswd_test.go | 63 +++++++++++++++++++++++ 1 file changed, 63 insertions(+) diff --git a/pkg/authentication/basic/htpasswd_test.go b/pkg/authentication/basic/htpasswd_test.go index 0c2a197e56..58237fb11a 100644 --- a/pkg/authentication/basic/htpasswd_test.go +++ b/pkg/authentication/basic/htpasswd_test.go @@ -1,6 +1,9 @@ package basic import ( + "io/ioutil" + "os" + . "github.com/onsi/ginkgo" . "github.com/onsi/gomega" ) @@ -91,6 +94,66 @@ var _ = Describe("HTPasswd Suite", func() { Expect(validator).To(BeNil()) }) }) + + Context("htpasswd file is updated", func() { + const filePathPrefix = "htpasswd-file-updated-" + const adminUserHtpasswdEntry = "admin:$2y$05$SXWrNM7ldtbRzBvUC3VXyOvUeiUcP45XPwM93P5eeGOEPIiAZmJjC" + const user1HtpasswdEntry = "user1:$2y$05$/sZYJOk8.3Etg4V6fV7puuXfCJLmV5Q7u3xvKpjBSJUka.t2YtmmG" + + assertHtpasswdMapChange := func(entryOne, entryTwo string, remove bool) *htpasswdMap { + var validator Validator + var file *os.File + var fileMask = os.O_RDWR | os.O_APPEND + var err error + + // Create a temporary file with at least one entry + file, err = ioutil.TempFile("", filePathPrefix) + Expect(err).ToNot(HaveOccurred()) + _, err = file.WriteString(entryOne + "\n") + Expect(err).ToNot(HaveOccurred()) + err = file.Close() + Expect(err).ToNot(HaveOccurred()) + + validator, err = NewHTPasswdValidator(file.Name()) + Expect(err).ToNot(HaveOccurred()) + + htpasswd, ok := validator.(*htpasswdMap) + Expect(ok).To(BeTrue()) + + // Remove previous htpasswd entries + if remove { + fileMask = os.O_RDWR | os.O_TRUNC + } + + // Add a new entry to the file in order to trigger an update + file, err = os.OpenFile(file.Name(), fileMask, 0644) + Expect(err).ToNot(HaveOccurred()) + _, err = file.WriteString(entryTwo + "\n") + Expect(err).ToNot(HaveOccurred()) + err = file.Close() + Expect(err).ToNot(HaveOccurred()) + + // Remove the temporary file created + err = os.Remove(file.Name()) + Expect(err).ToNot(HaveOccurred()) + + return htpasswd + } + + htpasswdAdd := assertHtpasswdMapChange(adminUserHtpasswdEntry, user1HtpasswdEntry, false) + It("htpasswdMap entry is present", func() { + Expect(len(htpasswdAdd.users)).To(Equal(2)) + Expect(htpasswdAdd.Validate(user1, user1Password)).To(BeTrue()) + Expect(htpasswdAdd.Validate(adminUser, adminPassword)).To(BeTrue()) + }) + + htpasswdRemove := assertHtpasswdMapChange(adminUserHtpasswdEntry, user1HtpasswdEntry, true) + It("htpasswdMap entry is not present", func() { + Expect(len(htpasswdRemove.users)).To(Equal(1)) + Expect(htpasswdRemove.Validate(user1, user1Password)).To(BeTrue()) + Expect(htpasswdRemove.Validate(adminUser, adminPassword)).To(BeFalse()) + }) + }) }) }) }) From 0a159e29fa54de6d7847f6892810ea8055d8a47b Mon Sep 17 00:00:00 2001 From: Alexandru Ciobanu Date: Sat, 25 Jun 2022 17:12:59 +0300 Subject: [PATCH 3/9] refactored `htpasswd` and `watcher` to lower cognitive complexity --- pkg/authentication/basic/htpasswd.go | 34 ++++++--- pkg/authentication/basic/htpasswd_test.go | 50 ++++++------ pkg/watcher/watcher.go | 93 +++++++++++------------ 3 files changed, 92 insertions(+), 85 deletions(-) diff --git a/pkg/authentication/basic/htpasswd.go b/pkg/authentication/basic/htpasswd.go index f16097c9d1..f2e871fc34 100644 --- a/pkg/authentication/basic/htpasswd.go +++ b/pkg/authentication/basic/htpasswd.go @@ -8,7 +8,6 @@ import ( "fmt" "io" "os" - "strings" "sync" "github.com/oauth2-proxy/oauth2-proxy/v7/pkg/logger" @@ -44,7 +43,7 @@ func NewHTPasswdValidator(path string) (Validator, error) { watcher.WatchFileForUpdates(path, nil, func() { err := h.loadHTPasswdFile(path) if err != nil { - logger.Error(err) + logger.Errorf("%v: no changes were made to the current htpasswd map", err) } }) @@ -98,16 +97,7 @@ func createHtpasswdMap(records [][]string) (*htpasswdMap, error) { switch { case lr == 2: user, realPassword := record[0], record[1] - if strings.HasPrefix(realPassword, "{SHA}") { - h.users[user] = sha1Pass(realPassword[5:]) - } else if strings.HasPrefix(realPassword, "$2b$") || - strings.HasPrefix(realPassword, "$2y$") || - strings.HasPrefix(realPassword, "$2x$") || - strings.HasPrefix(realPassword, "$2a$") { - h.users[user] = bcryptPass(realPassword) - } else { - invalidEntries = append(invalidEntries, user) - } + invalidEntries = passShaOrBcrypt(h, user, realPassword) case lr == 1, lr > 2: invalidRecords = append(invalidRecords, record[0]) } @@ -128,6 +118,26 @@ func createHtpasswdMap(records [][]string) (*htpasswdMap, error) { return h, nil } +// passShaOrBcrypt checks if a htpasswd entry is valid and the password is encrypted with SHA or bcrypt. +// Valid user entries are saved in the htpasswdMap, invalid records are reurned. +func passShaOrBcrypt(h *htpasswdMap, user, password string) (invalidEntries []string) { + passLen := len(password) + switch { + case passLen > 6 && password[:5] == "{SHA}": + h.users[user] = sha1Pass(password[5:]) + case passLen > 5 && + (password[:4] == "$2b$" || + password[:4] == "$2y$" || + password[:4] == "$2x$" || + password[:4] == "$2a$"): + h.users[user] = bcryptPass(password) + default: + invalidEntries = append(invalidEntries, user) + } + + return invalidEntries +} + // Validate checks a users password against the htpasswd entries func (h *htpasswdMap) Validate(user string, password string) bool { realPassword, exists := h.users[user] diff --git a/pkg/authentication/basic/htpasswd_test.go b/pkg/authentication/basic/htpasswd_test.go index 58237fb11a..cd1907964b 100644 --- a/pkg/authentication/basic/htpasswd_test.go +++ b/pkg/authentication/basic/htpasswd_test.go @@ -1,7 +1,6 @@ package basic import ( - "io/ioutil" "os" . "github.com/onsi/ginkgo" @@ -99,19 +98,25 @@ var _ = Describe("HTPasswd Suite", func() { const filePathPrefix = "htpasswd-file-updated-" const adminUserHtpasswdEntry = "admin:$2y$05$SXWrNM7ldtbRzBvUC3VXyOvUeiUcP45XPwM93P5eeGOEPIiAZmJjC" const user1HtpasswdEntry = "user1:$2y$05$/sZYJOk8.3Etg4V6fV7puuXfCJLmV5Q7u3xvKpjBSJUka.t2YtmmG" + var fileNames []string - assertHtpasswdMapChange := func(entryOne, entryTwo string, remove bool) *htpasswdMap { + AfterSuite(func() { + for _, v := range fileNames { + err := os.Remove(v) + Expect(err).ToNot(HaveOccurred()) + } + + }) + + htpasswdMap := func(entry, otherEntry string, remove bool) *htpasswdMap { var validator Validator var file *os.File - var fileMask = os.O_RDWR | os.O_APPEND var err error // Create a temporary file with at least one entry - file, err = ioutil.TempFile("", filePathPrefix) + file, err = os.CreateTemp("", filePathPrefix) Expect(err).ToNot(HaveOccurred()) - _, err = file.WriteString(entryOne + "\n") - Expect(err).ToNot(HaveOccurred()) - err = file.Close() + _, err = file.WriteString(entry + "\n") Expect(err).ToNot(HaveOccurred()) validator, err = NewHTPasswdValidator(file.Name()) @@ -120,39 +125,38 @@ var _ = Describe("HTPasswd Suite", func() { htpasswd, ok := validator.(*htpasswdMap) Expect(ok).To(BeTrue()) - // Remove previous htpasswd entries if remove { - fileMask = os.O_RDWR | os.O_TRUNC + // Overwrite the original file with another entry + err = os.WriteFile(file.Name(), []byte(otherEntry+"\n"), 0644) + Expect(err).ToNot(HaveOccurred()) + } else { + // Add another entry to the original file in append mode + _, err = file.WriteString(otherEntry + "\n") + Expect(err).ToNot(HaveOccurred()) } - // Add a new entry to the file in order to trigger an update - file, err = os.OpenFile(file.Name(), fileMask, 0644) - Expect(err).ToNot(HaveOccurred()) - _, err = file.WriteString(entryTwo + "\n") - Expect(err).ToNot(HaveOccurred()) err = file.Close() Expect(err).ToNot(HaveOccurred()) - // Remove the temporary file created - err = os.Remove(file.Name()) - Expect(err).ToNot(HaveOccurred()) + fileNames = append(fileNames, file.Name()) return htpasswd } - htpasswdAdd := assertHtpasswdMapChange(adminUserHtpasswdEntry, user1HtpasswdEntry, false) - It("htpasswdMap entry is present", func() { + htpasswdAdd := htpasswdMap(adminUserHtpasswdEntry, user1HtpasswdEntry, false) + It("htpasswd entry is added", func() { Expect(len(htpasswdAdd.users)).To(Equal(2)) - Expect(htpasswdAdd.Validate(user1, user1Password)).To(BeTrue()) Expect(htpasswdAdd.Validate(adminUser, adminPassword)).To(BeTrue()) + Expect(htpasswdAdd.Validate(user1, user1Password)).To(BeTrue()) }) - htpasswdRemove := assertHtpasswdMapChange(adminUserHtpasswdEntry, user1HtpasswdEntry, true) - It("htpasswdMap entry is not present", func() { + htpasswdRemove := htpasswdMap(adminUserHtpasswdEntry, user1HtpasswdEntry, true) + It("htpasswd entry is removed", func() { Expect(len(htpasswdRemove.users)).To(Equal(1)) - Expect(htpasswdRemove.Validate(user1, user1Password)).To(BeTrue()) Expect(htpasswdRemove.Validate(adminUser, adminPassword)).To(BeFalse()) + Expect(htpasswdRemove.Validate(user1, user1Password)).To(BeTrue()) }) + }) }) }) diff --git a/pkg/watcher/watcher.go b/pkg/watcher/watcher.go index 845c484a1e..e80a674f6b 100644 --- a/pkg/watcher/watcher.go +++ b/pkg/watcher/watcher.go @@ -10,73 +10,28 @@ import ( "github.com/oauth2-proxy/oauth2-proxy/v7/pkg/logger" ) -const writeOrCreateMask = fsnotify.Write | fsnotify.Create - -// WaitForReplacement waits for a file to exist on disk and then starts a watch -// for the file -func WaitForReplacement(filename string, op fsnotify.Op, - watcher *fsnotify.Watcher) { - const sleepInterval = 50 * time.Millisecond - - // Avoid a race when fsnofity.Remove is preceded by fsnotify.Chmod. - if op&fsnotify.Chmod != 0 { - time.Sleep(sleepInterval) - } - for { - if _, err := os.Stat(filename); err == nil { - if err := watcher.Add(filename); err == nil { - logger.Printf("watching resumed for '%s'", filename) - return - } - } - time.Sleep(sleepInterval) - } -} - // WatchForUpdates performs an action every time a file on disk is updated func WatchFileForUpdates(filename string, done <-chan bool, action func()) { - realFile, _ := filepath.EvalSymlinks(filename) + filename = filepath.Clean(filename) watcher, err := fsnotify.NewWatcher() if err != nil { logger.Fatalf("failed to create watcher for '%s': %s", filename, err) } go func() { - defer func(w *fsnotify.Watcher) { - cerr := w.Close() - if cerr != nil { - logger.Fatalf("error closing watcher: %v", err) - } - }(watcher) + defer watcher.Close() + for { select { case <-done: logger.Printf("shutting down watcher for: %s", filename) return case event, ok := <-watcher.Events: - // 'Events' channel is closed - if !ok { + if !ok { // 'Events' channel is closed logger.Errorf("error: cannot start the watcher, events channel is closed") return } - currentFile, _ := filepath.EvalSymlinks(filename) - // we only care about the config file with the following cases: - // 1 - if the file was modified or created - // 2 - if the real path to the file changed (eg: k8s ConfigMap/Secret replacement) - if (filepath.Clean(event.Name) == filename && - event.Op&writeOrCreateMask != 0) || - (currentFile != "" && currentFile != realFile) { - logger.Printf("reloading after event: %s", event) - realFile = currentFile - action() - } else if filepath.Clean(event.Name) == filename && - event.Op&fsnotify.Remove != 0 { - logger.Printf("watching interrupted on event: %s", event) - WaitForReplacement(filename, event.Op, watcher) - if err = watcher.Add(filename); err != nil { - logger.Fatalf("failed to add '%s' to watcher: %v", filename, err) - } - } + filterEvent(watcher, event, filename, action) case err = <-watcher.Errors: logger.Errorf("error watching '%s': %s", filename, err) } @@ -87,3 +42,41 @@ func WatchFileForUpdates(filename string, done <-chan bool, action func()) { } logger.Printf("watching '%s' for updates", filename) } + +// Filter file operations based on the events sent by the watcher. +// Execute the action() function when the following conditions are met: +// - the file is modified or created +// - the real path of the file was changed (Kubernetes ConfigMap/Secret) +func filterEvent(watcher *fsnotify.Watcher, event fsnotify.Event, filename string, action func()) { + switch filepath.Clean(event.Name) == filename { + case event.Op&(fsnotify.Remove|fsnotify.Rename) != 0: + logger.Printf("watching interrupted on event: %s", event) + WaitForReplacement(filename, event.Op, watcher) + action() + case event.Op&(fsnotify.Create|fsnotify.Write) != 0: + logger.Printf("reloading after event: %s", event) + action() + default: + logger.Printf("current event: %s", event) + } +} + +// WaitForReplacement waits for a file to exist on disk and then starts a watch +// for the file +func WaitForReplacement(filename string, op fsnotify.Op, watcher *fsnotify.Watcher) { + const sleepInterval = 50 * time.Millisecond + + // Avoid a race when fsnofity.Remove is preceded by fsnotify.Chmod. + if op&fsnotify.Chmod != 0 { + time.Sleep(sleepInterval) + } + for { + if _, err := os.Stat(filename); err == nil { + if err := watcher.Add(filename); err == nil { + logger.Printf("watching resumed for '%s'", filename) + return + } + } + time.Sleep(sleepInterval) + } +} From 07f18ce0b05903421309a810e4749dfd4232acc0 Mon Sep 17 00:00:00 2001 From: Alexandru Ciobanu Date: Fri, 22 Jul 2022 11:35:10 +0300 Subject: [PATCH 4/9] returned errors and refactored tests --- pkg/authentication/basic/htpasswd.go | 9 +++-- pkg/authentication/basic/htpasswd_test.go | 44 ++++++++++++++--------- pkg/watcher/watcher.go | 19 +++++----- 3 files changed, 45 insertions(+), 27 deletions(-) diff --git a/pkg/authentication/basic/htpasswd.go b/pkg/authentication/basic/htpasswd.go index f2e871fc34..9676b541df 100644 --- a/pkg/authentication/basic/htpasswd.go +++ b/pkg/authentication/basic/htpasswd.go @@ -40,12 +40,15 @@ func NewHTPasswdValidator(path string) (Validator, error) { return nil, err } - watcher.WatchFileForUpdates(path, nil, func() { + err = watcher.WatchFileForUpdates(path, nil, func() { err := h.loadHTPasswdFile(path) if err != nil { logger.Errorf("%v: no changes were made to the current htpasswd map", err) } }) + if err != nil { + return nil, err + } return h, nil } @@ -71,7 +74,7 @@ func (h *htpasswdMap) loadHTPasswdFile(filename string) error { records, err := csvReader.ReadAll() if err != nil { - logger.Fatalf("could not read htpasswd file: %v", err) + return fmt.Errorf("could not read htpasswd file: %v", err) } updated, err := createHtpasswdMap(records) @@ -112,7 +115,7 @@ func createHtpasswdMap(records [][]string) (*htpasswdMap, error) { } if len(h.users) == 0 { - logger.Fatal("could not construct htpasswdMap: htpasswd file doesn't contain a single valid user entry") + return nil, fmt.Errorf("could not construct htpasswdMap: htpasswd file doesn't contain a single valid user entry") } return h, nil diff --git a/pkg/authentication/basic/htpasswd_test.go b/pkg/authentication/basic/htpasswd_test.go index cd1907964b..70641d0346 100644 --- a/pkg/authentication/basic/htpasswd_test.go +++ b/pkg/authentication/basic/htpasswd_test.go @@ -5,6 +5,7 @@ import ( . "github.com/onsi/ginkgo" . "github.com/onsi/gomega" + . "github.com/onsi/gomega/types" ) const ( @@ -108,7 +109,14 @@ var _ = Describe("HTPasswd Suite", func() { }) - htpasswdMap := func(entry, otherEntry string, remove bool) *htpasswdMap { + type htpasswdUpdate struct { + testText string + remove bool + expectedLen int + expectedGomegaMatcher GomegaMatcher + } + + assertHtpasswdMapUpdate := func(hu htpasswdUpdate) { var validator Validator var file *os.File var err error @@ -116,7 +124,7 @@ var _ = Describe("HTPasswd Suite", func() { // Create a temporary file with at least one entry file, err = os.CreateTemp("", filePathPrefix) Expect(err).ToNot(HaveOccurred()) - _, err = file.WriteString(entry + "\n") + _, err = file.WriteString(adminUserHtpasswdEntry + "\n") Expect(err).ToNot(HaveOccurred()) validator, err = NewHTPasswdValidator(file.Name()) @@ -125,13 +133,13 @@ var _ = Describe("HTPasswd Suite", func() { htpasswd, ok := validator.(*htpasswdMap) Expect(ok).To(BeTrue()) - if remove { + if hu.remove { // Overwrite the original file with another entry - err = os.WriteFile(file.Name(), []byte(otherEntry+"\n"), 0644) + err = os.WriteFile(file.Name(), []byte(user1HtpasswdEntry+"\n"), 0644) Expect(err).ToNot(HaveOccurred()) } else { // Add another entry to the original file in append mode - _, err = file.WriteString(otherEntry + "\n") + _, err = file.WriteString(user1HtpasswdEntry + "\n") Expect(err).ToNot(HaveOccurred()) } @@ -140,21 +148,25 @@ var _ = Describe("HTPasswd Suite", func() { fileNames = append(fileNames, file.Name()) - return htpasswd + It("has the correct number of users", func() { + Expect(len(htpasswd.users)).To(Equal(hu.expectedLen)) + }) + + It(hu.testText, func() { + Expect(htpasswd.Validate(adminUser, adminPassword)).To(hu.expectedGomegaMatcher) + }) + + It("new entry is present", func() { + Expect(htpasswd.Validate(user1, user1Password)).To(BeTrue()) + }) } - htpasswdAdd := htpasswdMap(adminUserHtpasswdEntry, user1HtpasswdEntry, false) - It("htpasswd entry is added", func() { - Expect(len(htpasswdAdd.users)).To(Equal(2)) - Expect(htpasswdAdd.Validate(adminUser, adminPassword)).To(BeTrue()) - Expect(htpasswdAdd.Validate(user1, user1Password)).To(BeTrue()) + Context("htpasswd entry is added", func() { + assertHtpasswdMapUpdate(htpasswdUpdate{"initial entry is present", false, 2, BeTrue()}) }) - htpasswdRemove := htpasswdMap(adminUserHtpasswdEntry, user1HtpasswdEntry, true) - It("htpasswd entry is removed", func() { - Expect(len(htpasswdRemove.users)).To(Equal(1)) - Expect(htpasswdRemove.Validate(adminUser, adminPassword)).To(BeFalse()) - Expect(htpasswdRemove.Validate(user1, user1Password)).To(BeTrue()) + Context("htpasswd entry is removed", func() { + assertHtpasswdMapUpdate(htpasswdUpdate{"initial entry is removed", true, 1, BeFalse()}) }) }) diff --git a/pkg/watcher/watcher.go b/pkg/watcher/watcher.go index e80a674f6b..66eac2153b 100644 --- a/pkg/watcher/watcher.go +++ b/pkg/watcher/watcher.go @@ -1,6 +1,7 @@ package watcher import ( + "fmt" "os" "path/filepath" "time" @@ -10,12 +11,12 @@ import ( "github.com/oauth2-proxy/oauth2-proxy/v7/pkg/logger" ) -// WatchForUpdates performs an action every time a file on disk is updated -func WatchFileForUpdates(filename string, done <-chan bool, action func()) { +// WatchFileForUpdates performs an action every time a file on disk is updated +func WatchFileForUpdates(filename string, done <-chan bool, action func()) error { filename = filepath.Clean(filename) watcher, err := fsnotify.NewWatcher() if err != nil { - logger.Fatalf("failed to create watcher for '%s': %s", filename, err) + return fmt.Errorf("failed to create watcher for '%s': %s", filename, err) } go func() { @@ -38,26 +39,28 @@ func WatchFileForUpdates(filename string, done <-chan bool, action func()) { } }() if err = watcher.Add(filename); err != nil { - logger.Fatalf("failed to add '%s' to watcher: %v", filename, err) + return fmt.Errorf("failed to add '%s' to watcher: %v", filename, err) } logger.Printf("watching '%s' for updates", filename) + + return nil } // Filter file operations based on the events sent by the watcher. // Execute the action() function when the following conditions are met: -// - the file is modified or created // - the real path of the file was changed (Kubernetes ConfigMap/Secret) +// - the file is modified or created func filterEvent(watcher *fsnotify.Watcher, event fsnotify.Event, filename string, action func()) { switch filepath.Clean(event.Name) == filename { - case event.Op&(fsnotify.Remove|fsnotify.Rename) != 0: + // In Kubernetes the file path is a symlink, so we should take action + // when the ConfigMap/Secret is replaced. + case event.Op&fsnotify.Remove != 0: logger.Printf("watching interrupted on event: %s", event) WaitForReplacement(filename, event.Op, watcher) action() case event.Op&(fsnotify.Create|fsnotify.Write) != 0: logger.Printf("reloading after event: %s", event) action() - default: - logger.Printf("current event: %s", event) } } From 3eba5d64147bd2fc142d21cfcc1cf97507cd3516 Mon Sep 17 00:00:00 2001 From: Alexandru Ciobanu Date: Wed, 17 Aug 2022 13:59:38 +0300 Subject: [PATCH 5/9] added `CHANGELOG.md` entry for #1701 and fixed the codeclimate issue --- CHANGELOG.md | 2 ++ pkg/watcher/watcher.go | 6 +----- 2 files changed, 3 insertions(+), 5 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index f016f86f1e..82f3b486f8 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -11,6 +11,8 @@ - [#1669](https://github.com/oauth2-proxy/oauth2-proxy/pull/1699) Fix method deprecated error in lint (@t-katsumura) +- [#1701](https://github.com/oauth2-proxy/oauth2-proxy/pull/1701) Watch the htpasswd file for changes and update the htpasswdMap (@aiciobanu) + - [#1709](https://github.com/oauth2-proxy/oauth2-proxy/pull/1709) Show an alert message when basic auth credentials are invalid (@aiciobanu) - [#1720](https://github.com/oauth2-proxy/oauth2-proxy/pull/1720) Extract roles from authToken, to allow using allowed roles with Keycloak. diff --git a/pkg/watcher/watcher.go b/pkg/watcher/watcher.go index 66eac2153b..21d905b7d6 100644 --- a/pkg/watcher/watcher.go +++ b/pkg/watcher/watcher.go @@ -27,11 +27,7 @@ func WatchFileForUpdates(filename string, done <-chan bool, action func()) error case <-done: logger.Printf("shutting down watcher for: %s", filename) return - case event, ok := <-watcher.Events: - if !ok { // 'Events' channel is closed - logger.Errorf("error: cannot start the watcher, events channel is closed") - return - } + case event := <-watcher.Events: filterEvent(watcher, event, filename, action) case err = <-watcher.Errors: logger.Errorf("error watching '%s': %s", filename, err) From 12b19bf6c35b4c0d34e8130ea1cbb4dae1610a86 Mon Sep 17 00:00:00 2001 From: Alexandru Ciobanu <67386493+aiciobanu@users.noreply.github.com> Date: Thu, 1 Sep 2022 13:58:54 +0300 Subject: [PATCH 6/9] Apply suggestions from code review Co-authored-by: Joel Speed --- pkg/authentication/basic/htpasswd.go | 8 +++----- pkg/watcher/watcher.go | 2 +- 2 files changed, 4 insertions(+), 6 deletions(-) diff --git a/pkg/authentication/basic/htpasswd.go b/pkg/authentication/basic/htpasswd.go index 9676b541df..faedf46f5d 100644 --- a/pkg/authentication/basic/htpasswd.go +++ b/pkg/authentication/basic/htpasswd.go @@ -35,18 +35,16 @@ type sha1Pass string func NewHTPasswdValidator(path string) (Validator, error) { h := &htpasswdMap{users: make(map[string]interface{})} - err := h.loadHTPasswdFile(path) - if err != nil { + if err := h.loadHTPasswdFile(path); err != nil { return nil, err } - err = watcher.WatchFileForUpdates(path, nil, func() { + if err := watcher.WatchFileForUpdates(path, nil, func() { err := h.loadHTPasswdFile(path) if err != nil { logger.Errorf("%v: no changes were made to the current htpasswd map", err) } - }) - if err != nil { + }); err != nil { return nil, err } diff --git a/pkg/watcher/watcher.go b/pkg/watcher/watcher.go index 21d905b7d6..6a8a60d586 100644 --- a/pkg/watcher/watcher.go +++ b/pkg/watcher/watcher.go @@ -34,7 +34,7 @@ func WatchFileForUpdates(filename string, done <-chan bool, action func()) error } } }() - if err = watcher.Add(filename); err != nil { + if err := watcher.Add(filename); err != nil { return fmt.Errorf("failed to add '%s' to watcher: %v", filename, err) } logger.Printf("watching '%s' for updates", filename) From 015fe2086663eb911059b39f7419c91bf8afcc4d Mon Sep 17 00:00:00 2001 From: Alexandru Ciobanu Date: Thu, 1 Sep 2022 14:09:07 +0300 Subject: [PATCH 7/9] Fix lint issue from code suggestion --- pkg/authentication/basic/htpasswd.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/pkg/authentication/basic/htpasswd.go b/pkg/authentication/basic/htpasswd.go index faedf46f5d..2bb6ff2335 100644 --- a/pkg/authentication/basic/htpasswd.go +++ b/pkg/authentication/basic/htpasswd.go @@ -44,7 +44,7 @@ func NewHTPasswdValidator(path string) (Validator, error) { if err != nil { logger.Errorf("%v: no changes were made to the current htpasswd map", err) } - }); err != nil { + }); err != nil { return nil, err } From 37fa6e7b027a117d3015a1986d3e5253ef3aef24 Mon Sep 17 00:00:00 2001 From: Alexandru Ciobanu Date: Thu, 1 Sep 2022 15:27:55 +0300 Subject: [PATCH 8/9] Wrap htpasswd load and watch errors with context --- oauthproxy.go | 2 +- pkg/authentication/basic/htpasswd.go | 4 ++-- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/oauthproxy.go b/oauthproxy.go index 63fae004da..c2c25a8c97 100644 --- a/oauthproxy.go +++ b/oauthproxy.go @@ -113,7 +113,7 @@ func NewOAuthProxy(opts *options.Options, validator func(string) bool) (*OAuthPr var err error basicAuthValidator, err = basic.NewHTPasswdValidator(opts.HtpasswdFile) if err != nil { - return nil, fmt.Errorf("could not load htpasswd file: %v", err) + return nil, fmt.Errorf("could not validate htpasswd: %v", err) } } diff --git a/pkg/authentication/basic/htpasswd.go b/pkg/authentication/basic/htpasswd.go index 2bb6ff2335..8a4ac2106c 100644 --- a/pkg/authentication/basic/htpasswd.go +++ b/pkg/authentication/basic/htpasswd.go @@ -36,7 +36,7 @@ func NewHTPasswdValidator(path string) (Validator, error) { h := &htpasswdMap{users: make(map[string]interface{})} if err := h.loadHTPasswdFile(path); err != nil { - return nil, err + return nil, fmt.Errorf("could not load htpasswd file: %v", err) } if err := watcher.WatchFileForUpdates(path, nil, func() { @@ -45,7 +45,7 @@ func NewHTPasswdValidator(path string) (Validator, error) { logger.Errorf("%v: no changes were made to the current htpasswd map", err) } }); err != nil { - return nil, err + return nil, fmt.Errorf("could not watch htpasswd file: %v", err) } return h, nil From 9e0c4689eacbef5bfae2472e0aa3c65e4b5a7188 Mon Sep 17 00:00:00 2001 From: Alexandru Ciobanu Date: Thu, 1 Sep 2022 15:37:31 +0300 Subject: [PATCH 9/9] add the htpasswd wrapped error context to the test --- pkg/authentication/basic/htpasswd_test.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/pkg/authentication/basic/htpasswd_test.go b/pkg/authentication/basic/htpasswd_test.go index 70641d0346..d5b4150f18 100644 --- a/pkg/authentication/basic/htpasswd_test.go +++ b/pkg/authentication/basic/htpasswd_test.go @@ -87,7 +87,7 @@ var _ = Describe("HTPasswd Suite", func() { }) It("returns an error", func() { - Expect(err).To(MatchError("could not open htpasswd file: open ./test/htpasswd-doesnt-exist.txt: no such file or directory")) + Expect(err).To(MatchError("could not load htpasswd file: could not open htpasswd file: open ./test/htpasswd-doesnt-exist.txt: no such file or directory")) }) It("returns a nil validator", func() {