Repository: oauth2-proxy/oauth2-proxy
Pull request #687 (2020-07-18 09:22:44)

Refactor HTPasswd Validator
# Description

Refactor the HTPasswdValidator and move to an `authentication/basic` package.

## Motivation and Context

<!--- Why is this change required? What problem does it solve? -->
<!--- If it fixes an open issue, please link to the issue here. -->
This improves the testing of the HTPasswd validation and also determines at start up what type of password users have so that runtime efficiency of the password lookups should be improved.

## How Has This Been Tested?

<!--- Please describe in detail how you tested your changes. -->
<!--- Include details of your testing environment, and the tests you ran to -->
<!--- see how your change affects other areas of the code, etc. -->
Unit tests.

## Checklist:

<!--- Go over all the following points, and put an `x` in all the boxes that apply. -->
<!--- If you're unsure about any of these, don't hesitate to ask. We're here to help! -->

- [x] My change requires a change to the documentation or CHANGELOG.
- [x] I have updated the documentation/CHANGELOG accordingly.
- [x] I have created a feature (non-master) branch for my PR.



Patch:
diff --git a/CHANGELOG.md b/CHANGELOG.md
index 62344892..ad30c6ae 100644
--- a/CHANGELOG.md
+++ b/CHANGELOG.md
@@ -11,6 +11,7 @@
 
 ## Changes since v6.0.0
 
+- [#687](https://github.com/oauth2-proxy/oauth2-proxy/pull/687) Refactor HTPasswd Validator (@JoelSpeed)
 - [#624](https://github.com/oauth2-proxy/oauth2-proxy/pull/624) Allow stripping authentication headers from whitelisted requests with `--skip-auth-strip-headers` (@NickMeves)
 - [#673](https://github.com/oauth2-proxy/oauth2-proxy/pull/673) Add --session-cookie-minimal option to create session cookies with no tokens (@NickMeves)
 - [#632](https://github.com/oauth2-proxy/oauth2-proxy/pull/632) Reduce session size by encoding with MessagePack and using LZ4 compression (@NickMeves)
diff --git a/htpasswd.go b/htpasswd.go
deleted file mode 100644
index 670aa729..00000000
--- a/htpasswd.go
+++ /dev/null
@@ -1,72 +0,0 @@
-package main
-
-import (
-	"crypto/sha1"
-	"encoding/base64"
-	"encoding/csv"
-	"io"
-	"os"
-
-	"github.com/oauth2-proxy/oauth2-proxy/pkg/logger"
-	"golang.org/x/crypto/bcrypt"
-)
-
-// Lookup passwords in a htpasswd file
-// Passwords must be generated with -B for bcrypt or -s for SHA1.
-
-// HtpasswdFile represents the structure of an htpasswd file
-type HtpasswdFile struct {
-	Users map[string]string
-}
-
-// NewHtpasswdFromFile constructs an HtpasswdFile from the file at the path given
-func NewHtpasswdFromFile(path string) (*HtpasswdFile, error) {
-	r, err := os.Open(path)
-	if err != nil {
-		return nil, err
-	}
-	defer r.Close()
-	return NewHtpasswd(r)
-}
-
-// NewHtpasswd  consctructs an HtpasswdFile from an io.Reader (opened file)
-func NewHtpasswd(file io.Reader) (*HtpasswdFile, error) {
-	csvReader := csv.NewReader(file)
-	csvReader.Comma = ':'
-	csvReader.Comment = '#'
-	csvReader.TrimLeadingSpace = true
-
-	records, err := csvReader.ReadAll()
-	if err != nil {
-		return nil, err
-	}
-	h := &HtpasswdFile{Users: make(map[string]string)}
-	for _, record := range records {
-		h.Users[record[0]] = record[1]
-	}
-	return h, nil
-}
-
-// Validate checks a users password against the HtpasswdFile entries
-func (h *HtpasswdFile) Validate(user string, password string) bool {
-	realPassword, exists := h.Users[user]
-	if !exists {
-		return false
-	}
-
-	shaPrefix := realPassword[:5]
-	if shaPrefix == "{SHA}" {
-		shaValue := realPassword[5:]
-		d := sha1.New()
-		d.Write([]byte(password))
-		return shaValue == base64.StdEncoding.EncodeToString(d.Sum(nil))
-	}
-
-	bcryptPrefix := realPassword[:4]
-	if bcryptPrefix == "$2a$" || bcryptPrefix == "$2b$" || bcryptPrefix == "$2x$" || bcryptPrefix == "$2y$" {
-		return bcrypt.CompareHashAndPassword([]byte(realPassword), []byte(password)) == nil
-	}
-
-	logger.Printf("Invalid htpasswd entry for %s. Must be a SHA or bcrypt entry.", user)
-	return false
-}
diff --git a/main.go b/main.go
index 4ee6af4b..b42c2c05 100644
--- a/main.go
+++ b/main.go
@@ -66,15 +66,6 @@ func main() {
 		}
 	}
 
-	if opts.HtpasswdFile != "" {
-		logger.Printf("using htpasswd file %s", opts.HtpasswdFile)
-		oauthproxy.HtpasswdFile, err = NewHtpasswdFromFile(opts.HtpasswdFile)
-		oauthproxy.DisplayHtpasswdForm = opts.DisplayHtpasswdForm
-		if err != nil {
-			logger.Fatalf("FATAL: unable to open %s %s", opts.HtpasswdFile, err)
-		}
-	}
-
 	rand.Seed(time.Now().UnixNano())
 
 	chain := alice.New()
diff --git a/oauthproxy.go b/oauthproxy.go
index c05c793d..4b310b9b 100644
--- a/oauthproxy.go
+++ b/oauthproxy.go
@@ -22,6 +22,7 @@ import (
 	ipapi "github.com/oauth2-proxy/oauth2-proxy/pkg/apis/ip"
 	"github.com/oauth2-proxy/oauth2-proxy/pkg/apis/options"
 	sessionsapi "github.com/oauth2-proxy/oauth2-proxy/pkg/apis/sessions"
+	"github.com/oauth2-proxy/oauth2-proxy/pkg/authentication/basic"
 	"github.com/oauth2-proxy/oauth2-proxy/pkg/cookies"
 	"github.com/oauth2-proxy/oauth2-proxy/pkg/encryption"
 	"github.com/oauth2-proxy/oauth2-proxy/pkg/ip"
@@ -96,8 +97,8 @@ type OAuthProxy struct {
 	sessionStore            sessionsapi.SessionStore
 	ProxyPrefix             string
 	SignInMessage           string
-	HtpasswdFile            *HtpasswdFile
-	DisplayHtpasswdForm     bool
+	basicAuthValidator      basic.Validator
+	displayHtpasswdForm     bool
 	serveMux                http.Handler
 	SetXAuthRequest         bool
 	PassBasicAuth           bool
@@ -314,6 +315,16 @@ func NewOAuthProxy(opts *options.Options, validator func(string) bool) (*OAuthPr
 		}
 	}
 
+	var basicAuthValidator basic.Validator
+	if opts.HtpasswdFile != "" {
+		logger.Printf("using htpasswd file: %s", opts.HtpasswdFile)
+		var err error
+		basicAuthValidator, err = basic.NewHTPasswdValidator(opts.HtpasswdFile)
+		if err != nil {
+			return nil, fmt.Errorf("could not load htpasswdfile: %v", err)
+		}
+	}
+
 	return &OAuthProxy{
 		CookieName:     opts.Cookie.Name,
 		CSRFCookieName: fmt.Sprintf("%v_%v", opts.Cookie.Name, "csrf"),
@@ -364,6 +375,9 @@ func NewOAuthProxy(opts *options.Options, validator func(string) bool) (*OAuthPr
 		trustedIPs:              trustedIPs,
 		Banner:                  opts.Banner,
 		Footer:                  opts.Footer,
+
+		basicAuthValidator:  basicAuthValidator,
+		displayHtpasswdForm: basicAuthValidator != nil,
 	}, nil
 }
 
@@ -386,10 +400,6 @@ func (p *OAuthProxy) GetRedirectURI(host string) string {
 	return u.String()
 }
 
-func (p *OAuthProxy) displayCustomLoginForm() bool {
-	return p.HtpasswdFile != nil && p.DisplayHtpasswdForm
-}
-
 func (p *OAuthProxy) redeemCode(ctx context.Context, host, code string) (s *sessionsapi.SessionState, err error) {
 	if code == "" {
 		return nil, errors.New("missing code")
@@ -526,7 +536,7 @@ func (p *OAuthProxy) SignInPage(rw http.ResponseWriter, req *http.Request, code
 	}{
 		ProviderName:  p.provider.Data().ProviderName,
 		SignInMessage: template.HTML(p.SignInMessage),
-		CustomLogin:   p.displayCustomLoginForm(),
+		CustomLogin:   p.displayHtpasswdForm,
 		Redirect:      redirectURL,
 		Version:       VERSION,
 		ProxyPrefix:   p.ProxyPrefix,
@@ -540,7 +550,7 @@ func (p *OAuthProxy) SignInPage(rw http.ResponseWriter, req *http.Request, code
 
 // ManualSignIn handles basic auth logins to the proxy
 func (p *OAuthProxy) ManualSignIn(rw http.ResponseWriter, req *http.Request) (string, bool) {
-	if req.Method != "POST" || p.HtpasswdFile == nil {
+	if req.Method != "POST" || p.basicAuthValidator == nil {
 		return "", false
 	}
 	user := req.FormValue("username")
@@ -549,7 +559,7 @@ func (p *OAuthProxy) ManualSignIn(rw http.ResponseWriter, req *http.Request) (st
 		return "", false
 	}
 	// check auth
-	if p.HtpasswdFile.Validate(user, passwd) {
+	if p.basicAuthValidator.Validate(user, passwd) {
 		logger.PrintAuthf(user, req, logger.AuthSuccess, "Authenticated via HtpasswdFile")
 		return user, true
 	}
@@ -1159,7 +1169,7 @@ func (p *OAuthProxy) stripAuthHeaders(req *http.Request) {
 // CheckBasicAuth checks the requests Authorization header for basic auth
 // credentials and authenticates these against the proxies HtpasswdFile
 func (p *OAuthProxy) CheckBasicAuth(req *http.Request) (*sessionsapi.SessionState, error) {
-	if p.HtpasswdFile == nil {
+	if p.basicAuthValidator == nil {
 		return nil, nil
 	}
 	auth := req.Header.Get("Authorization")
@@ -1178,7 +1188,7 @@ func (p *OAuthProxy) CheckBasicAuth(req *http.Request) (*sessionsapi.SessionStat
 	if len(pair) != 2 {
 		return nil, fmt.Errorf("invalid format %s", b)
 	}
-	if p.HtpasswdFile.Validate(pair[0], pair[1]) {
+	if p.basicAuthValidator.Validate(pair[0], pair[1]) {
 		logger.PrintAuthf(pair[0], req, logger.AuthSuccess, "Authenticated via basic auth and HTpasswd File")
 		return &sessionsapi.SessionState{User: pair[0]}, nil
 	}
diff --git a/pkg/authentication/basic/htpasswd.go b/pkg/authentication/basic/htpasswd.go
new file mode 100644
index 00000000..ff87894f
--- /dev/null
+++ b/pkg/authentication/basic/htpasswd.go
@@ -0,0 +1,96 @@
+package basic
+
+import (
+	"crypto/sha1"
+	"encoding/base64"
+	"encoding/csv"
+	"fmt"
+	"io"
+	"os"
+
+	"github.com/oauth2-proxy/oauth2-proxy/pkg/logger"
+	"golang.org/x/crypto/bcrypt"
+)
+
+// htpasswdMap represents the structure of an htpasswd file.
+// Passwords must be generated with -B for bcrypt or -s for SHA1.
+type htpasswdMap struct {
+	users map[string]interface{}
+}
+
+// bcryptPass is used to identify bcrypt passwords in the
+// htpasswdMap users.
+type bcryptPass string
+
+// sha1Pass os used to identify sha1 passwords in the
+// htpasswdMap users.
+type sha1Pass string
+
+// NewHTPasswdValidator constructs an httpasswd based validator from the file
+// at the path given.
+func NewHTPasswdValidator(path string) (Validator, error) {
+	r, err := os.Open(path)
+	if err != nil {
+		return nil, fmt.Errorf("could not open htpasswd file: %v", err)
+	}
+	defer r.Close()
+	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.Comma = ':'
+	csvReader.Comment = '#'
+	csvReader.TrimLeadingSpace = true
+
+	records, err := csvReader.ReadAll()
+	if err != nil {
+		return nil, fmt.Errorf("could not read htpasswd file: %v", err)
+	}
+
+	return createHtpasswdMap(records)
+}
+
+// createHtasswdMap constructs an htpasswdMap from the given records
+func createHtpasswdMap(records [][]string) (*htpasswdMap, error) {
+	h := &htpasswdMap{users: make(map[string]interface{})}
+	for _, record := range records {
+		user, realPassword := record[0], record[1]
+		shaPrefix := realPassword[:5]
+		if shaPrefix == "{SHA}" {
+			h.users[user] = sha1Pass(realPassword[5:])
+			continue
+		}
+
+		bcryptPrefix := realPassword[:4]
+		if bcryptPrefix == "$2a$" || bcryptPrefix == "$2b$" || bcryptPrefix == "$2x$" || bcryptPrefix == "$2y$" {
+			h.users[user] = bcryptPass(realPassword)
+			continue
+		}
+
+		// Password is neither sha1 or bcrypt
+		// TODO(JoelSpeed): In the next breaking release, make this return an error.
+		logger.Printf("Invalid htpasswd entry for %s. Must be a SHA or bcrypt entry.", user)
+	}
+	return h, nil
+}
+
+// Validate checks a users password against the htpasswd entries
+func (h *htpasswdMap) Validate(user string, password string) bool {
+	realPassword, exists := h.users[user]
+	if !exists {
+		return false
+	}
+
+	switch real := realPassword.(type) {
+	case sha1Pass:
+		d := sha1.New()
+		d.Write([]byte(password))
+		return string(real) == base64.StdEncoding.EncodeToString(d.Sum(nil))
+	case bcryptPass:
+		return bcrypt.CompareHashAndPassword([]byte(real), []byte(password)) == nil
+	default:
+		return false
+	}
+}
diff --git a/pkg/authentication/basic/validator.go b/pkg/authentication/basic/validator.go
new file mode 100644
index 00000000..7a1ceda9
--- /dev/null
+++ b/pkg/authentication/basic/validator.go
@@ -0,0 +1,7 @@
+package basic
+
+// Validator is a minimal interface for something that can validate a
+// username and password combination.
+type Validator interface {
+	Validate(user, password string) bool
+}


Test patch:
diff --git a/htpasswd_test.go b/htpasswd_test.go
deleted file mode 100644
index 7a043e46..00000000
--- a/htpasswd_test.go
+++ /dev/null
@@ -1,38 +0,0 @@
-package main
-
-import (
-	"bytes"
-	"fmt"
-	"testing"
-
-	"github.com/stretchr/testify/assert"
-	"golang.org/x/crypto/bcrypt"
-)
-
-func TestSHA(t *testing.T) {
-	file := bytes.NewBuffer([]byte("testuser:{SHA}PaVBVZkYqAjCQCu6UBL2xgsnZhw=\n"))
-	h, err := NewHtpasswd(file)
-	assert.Equal(t, err, nil)
-
-	valid := h.Validate("testuser", "asdf")
-	assert.Equal(t, valid, true)
-}
-
-func TestBcrypt(t *testing.T) {
-	hash1, err := bcrypt.GenerateFromPassword([]byte("password"), 1)
-	assert.Equal(t, err, nil)
-	hash2, err := bcrypt.GenerateFromPassword([]byte("top-secret"), 2)
-	assert.Equal(t, err, nil)
-
-	contents := fmt.Sprintf("testuser1:%s\ntestuser2:%s\n", hash1, hash2)
-	file := bytes.NewBuffer([]byte(contents))
-
-	h, err := NewHtpasswd(file)
-	assert.Equal(t, err, nil)
-
-	valid := h.Validate("testuser1", "password")
-	assert.Equal(t, valid, true)
-
-	valid = h.Validate("testuser2", "top-secret")
-	assert.Equal(t, valid, true)
-}
diff --git a/pkg/authentication/basic/basic_suite_test.go b/pkg/authentication/basic/basic_suite_test.go
new file mode 100644
index 00000000..4d5fa806
--- /dev/null
+++ b/pkg/authentication/basic/basic_suite_test.go
@@ -0,0 +1,16 @@
+package basic
+
+import (
+	"testing"
+
+	"github.com/oauth2-proxy/oauth2-proxy/pkg/logger"
+	. "github.com/onsi/ginkgo"
+	. "github.com/onsi/gomega"
+)
+
+func TestBasicSuite(t *testing.T) {
+	logger.SetOutput(GinkgoWriter)
+
+	RegisterFailHandler(Fail)
+	RunSpecs(t, "Basic")
+}
diff --git a/pkg/authentication/basic/htpasswd_test.go b/pkg/authentication/basic/htpasswd_test.go
new file mode 100644
index 00000000..0c2a197e
--- /dev/null
+++ b/pkg/authentication/basic/htpasswd_test.go
@@ -0,0 +1,96 @@
+package basic
+
+import (
+	. "github.com/onsi/ginkgo"
+	. "github.com/onsi/gomega"
+)
+
+const (
+	adminUser     = "admin"
+	adminPassword = "Adm1n1str$t0r"
+	user1         = "user1"
+	user1Password = "UsErOn3P455"
+	user2         = "user2"
+	user2Password = "us3r2P455W0Rd!"
+)
+
+var _ = Describe("HTPasswd Suite", func() {
+	Context("with an HTPassword Validator", func() {
+		assertHtpasswdMapFromFile := func(filePath string) {
+			var htpasswd *htpasswdMap
+			var err error
+
+			BeforeEach(func() {
+				var validator Validator
+				validator, err = NewHTPasswdValidator(filePath)
+
+				var ok bool
+				htpasswd, ok = validator.(*htpasswdMap)
+				Expect(ok).To(BeTrue())
+			})
+
+			It("does not return an error", func() {
+				Expect(err).ToNot(HaveOccurred())
+			})
+
+			It("has the correct number of users", func() {
+				Expect(htpasswd.users).To(HaveLen(3))
+			})
+
+			It("accepts the correct passwords", func() {
+				Expect(htpasswd.Validate(adminUser, adminPassword)).To(BeTrue())
+				Expect(htpasswd.Validate(user1, user1Password)).To(BeTrue())
+				Expect(htpasswd.Validate(user2, user2Password)).To(BeTrue())
+			})
+
+			It("rejects incorrect passwords", func() {
+				Expect(htpasswd.Validate(adminUser, "asvdfda")).To(BeFalse())
+				Expect(htpasswd.Validate(user1, "BHEdgbtr")).To(BeFalse())
+				Expect(htpasswd.Validate(user2, "12345")).To(BeFalse())
+			})
+
+			It("rejects a non existent user", func() {
+				// Users are case sensitive
+				Expect(htpasswd.Validate("ADMIN", adminPassword)).To(BeFalse())
+			})
+		}
+
+		Context("load from file", func() {
+			Context("with sha1 entries", func() {
+				const filePath = "./test/htpasswd-sha1.txt"
+
+				assertHtpasswdMapFromFile(filePath)
+			})
+
+			Context("with bcrypt entries", func() {
+				const filePath = "./test/htpasswd-bcrypt.txt"
+
+				assertHtpasswdMapFromFile(filePath)
+			})
+
+			Context("with mixed entries", func() {
+				const filePath = "./test/htpasswd-mixed.txt"
+
+				assertHtpasswdMapFromFile(filePath)
+			})
+
+			Context("with a non existent file", func() {
+				const filePath = "./test/htpasswd-doesnt-exist.txt"
+				var validator Validator
+				var err error
+
+				BeforeEach(func() {
+					validator, err = NewHTPasswdValidator(filePath)
+				})
+
+				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"))
+				})
+
+				It("returns a nil validator", func() {
+					Expect(validator).To(BeNil())
+				})
+			})
+		})
+	})
+})
diff --git a/pkg/authentication/basic/test/htpasswd-bcrypt.txt b/pkg/authentication/basic/test/htpasswd-bcrypt.txt
new file mode 100644
index 00000000..5fcaa381
--- /dev/null
+++ b/pkg/authentication/basic/test/htpasswd-bcrypt.txt
@@ -0,0 +1,8 @@
+# admin:Adm1n1str$t0r
+admin:$2y$05$SXWrNM7ldtbRzBvUC3VXyOvUeiUcP45XPwM93P5eeGOEPIiAZmJjC
+
+# user1:UsErOn3P455
+user1:$2y$05$/sZYJOk8.3Etg4V6fV7puuXfCJLmV5Q7u3xvKpjBSJUka.t2YtmmG
+
+# user2: us3r2P455W0Rd!
+user2:$2y$05$l22MubgKTZFTjTs8TNg5k.YKvcnM2.bA/.iwl0idef5CbekdvBxva
diff --git a/pkg/authentication/basic/test/htpasswd-mixed.txt b/pkg/authentication/basic/test/htpasswd-mixed.txt
new file mode 100644
index 00000000..fd66c575
--- /dev/null
+++ b/pkg/authentication/basic/test/htpasswd-mixed.txt
@@ -0,0 +1,8 @@
+# admin:Adm1n1str$t0r
+admin:$2y$05$SXWrNM7ldtbRzBvUC3VXyOvUeiUcP45XPwM93P5eeGOEPIiAZmJjC
+
+# user1:UsErOn3P455
+user1:{SHA}Dvs/L78raajL4jEAHPkwflQXJzI=
+
+# user2: us3r2P455W0Rd!
+user2:{SHA}MoN9/JCJEcYUb6GCQ+2buDvn9pI=
diff --git a/pkg/authentication/basic/test/htpasswd-sha1.txt b/pkg/authentication/basic/test/htpasswd-sha1.txt
new file mode 100644
index 00000000..beb1f0d2
--- /dev/null
+++ b/pkg/authentication/basic/test/htpasswd-sha1.txt
@@ -0,0 +1,8 @@
+# admin:Adm1n1str$t0r
+admin:{SHA}gXQeRH0bcaCfhAk2gOLm1uaePMA=
+
+# user1:UsErOn3P455
+user1:{SHA}Dvs/L78raajL4jEAHPkwflQXJzI=
+
+# user2: us3r2P455W0Rd!
+user2:{SHA}MoN9/JCJEcYUb6GCQ+2buDvn9pI=