Add failing test showing issue

Possible fix for #31

go.sum

@@ -55,6 +55,7 @@ github.com/stretchr/objx v0.2.0/go.mod h1:qt09Ya8vawLte6SNmTgCsAVtYtaKzEcn8ATUoH
 github.com/stretchr/testify v1.2.2/go.mod h1:a8OnRcib4nhh0OaRAV+Yts87kKdq0PP7pXfy6kDkUVs=
 github.com/stretchr/testify v1.3.0 h1:TivCn/peBQ7UY8ooIcPgZFpTNSz0Q2U6UrFlUfqbe0Q=
 github.com/stretchr/testify v1.3.0/go.mod h1:M5WIy9Dh21IEIfnGCwXGc5bZfKNJtfHm1UVUgZn+9EI=
+github.com/stretchr/testify v1.5.1 h1:nOGnQDM7FYENwehXlg/kFVnos3rEvtKTjRvOWSzb6H4=
 github.com/thomseddon/go-flags v1.4.0 h1:cHj56pbnQxlGo2lx2P8f0Dph4TRYKBJzoPuF2lqNvW4=
 github.com/thomseddon/go-flags v1.4.0/go.mod h1:NK9eZpNBmSKVxvyB/MExg6jW0Bo9hQyAuCP+b8MJFow=
 github.com/thomseddon/go-flags v1.4.1-0.20190507181358-ce437f05b7fb h1:L311/fJ7WXmFDDtuhf22PkVJqZpqLbEsmGSTEGv7ZQY=

internal/auth.go

@@ -18,41 +18,41 @@ import (
 // Request Validation
 
 // Cookie = hash(secret, cookie domain, email, expires)|expires|email
-func ValidateCookie(r *http.Request, c *http.Cookie) (bool, string, error) {
+func ValidateCookie(r *http.Request, c *http.Cookie) (string, error) {
 	parts := strings.Split(c.Value, "|")
 
 	if len(parts) != 3 {
-		return false, "", errors.New("Invalid cookie format")
+		return "", errors.New("Invalid cookie format")
 	}
 
 	mac, err := base64.URLEncoding.DecodeString(parts[0])
 	if err != nil {
-		return false, "", errors.New("Unable to decode cookie mac")
+		return "", errors.New("Unable to decode cookie mac")
 	}
 
 	expectedSignature := cookieSignature(r, parts[2], parts[1])
 	expected, err := base64.URLEncoding.DecodeString(expectedSignature)
 	if err != nil {
-		return false, "", errors.New("Unable to generate mac")
+		return "", errors.New("Unable to generate mac")
 	}
 
 	// Valid token?
 	if !hmac.Equal(mac, expected) {
-		return false, "", errors.New("Invalid cookie mac")
+		return "", errors.New("Invalid cookie mac")
 	}
 
 	expires, err := strconv.ParseInt(parts[1], 10, 64)
 	if err != nil {
-		return false, "", errors.New("Unable to parse cookie expiry")
+		return "", errors.New("Unable to parse cookie expiry")
 	}
 
 	// Has it expired?
 	if time.Unix(expires, 0).Before(time.Now()) {
-		return false, "", errors.New("Cookie has expired")
+		return "", errors.New("Cookie has expired")
 	}
 
 	// Looks valid
-	return true, parts[2], nil
+	return parts[2], nil
 }
 
 // Validate email

internal/auth_test.go

@@ -24,28 +24,24 @@ func TestAuthValidateCookie(t *testing.T) {
 
 	// Should require 3 parts
 	c.Value = ""
-	valid, _, err := ValidateCookie(r, c)
-	assert.False(valid)
+	_, err := ValidateCookie(r, c)
 	if assert.Error(err) {
 		assert.Equal("Invalid cookie format", err.Error())
 	}
 	c.Value = "1|2"
-	valid, _, err = ValidateCookie(r, c)
-	assert.False(valid)
+	_, err = ValidateCookie(r, c)
 	if assert.Error(err) {
 		assert.Equal("Invalid cookie format", err.Error())
 	}
 	c.Value = "1|2|3|4"
-	valid, _, err = ValidateCookie(r, c)
-	assert.False(valid)
+	_, err = ValidateCookie(r, c)
 	if assert.Error(err) {
 		assert.Equal("Invalid cookie format", err.Error())
 	}
 
 	// Should catch invalid mac
 	c.Value = "MQ==|2|3"
-	valid, _, err = ValidateCookie(r, c)
-	assert.False(valid)
+	_, err = ValidateCookie(r, c)
 	if assert.Error(err) {
 		assert.Equal("Invalid cookie mac", err.Error())
 	}
@@ -53,8 +49,7 @@ func TestAuthValidateCookie(t *testing.T) {
 	// Should catch expired
 	config.Lifetime = time.Second * time.Duration(-1)
 	c = MakeCookie(r, "test@test.com")
-	valid, _, err = ValidateCookie(r, c)
-	assert.False(valid)
+	_, err = ValidateCookie(r, c)
 	if assert.Error(err) {
 		assert.Equal("Cookie has expired", err.Error())
 	}
@@ -62,8 +57,7 @@ func TestAuthValidateCookie(t *testing.T) {
 	// Should accept valid cookie
 	config.Lifetime = time.Second * time.Duration(10)
 	c = MakeCookie(r, "test@test.com")
-	valid, email, err := ValidateCookie(r, c)
-	assert.True(valid, "valid request should return valid")
+	email, err := ValidateCookie(r, c)
 	assert.Nil(err, "valid request should not return an error")
 	assert.Equal("test@test.com", email, "valid request should return user email")
 }
@@ -244,8 +238,8 @@ func TestAuthMakeCookie(t *testing.T) {
 	assert.Equal("_forward_auth", c.Name)
 	parts := strings.Split(c.Value, "|")
 	assert.Len(parts, 3, "cookie should be 3 parts")
-	valid, _, _ := ValidateCookie(r, c)
-	assert.True(valid, "should generate valid cookie")
+	_, err := ValidateCookie(r, c)
+	assert.Nil(err, "should generate valid cookie")
 	assert.Equal("/", c.Path)
 	assert.Equal("app.example.com", c.Domain)
 	assert.True(c.Secure)

internal/config.go

@@ -31,7 +31,7 @@ type Config struct {
 	CookieName     string               `long:"cookie-name" env:"COOKIE_NAME" default:"_forward_auth" description:"Cookie Name"`
 	CSRFCookieName string               `long:"csrf-cookie-name" env:"CSRF_COOKIE_NAME" default:"_forward_auth_csrf" description:"CSRF Cookie Name"`
 	DefaultAction  string               `long:"default-action" env:"DEFAULT_ACTION" default:"auth" choice:"auth" choice:"allow" description:"Default action"`
-	Domains        []string             `long:"domain" env:"DOMAIN" description:"Only allow given email domains, can be set multiple times"`
+	Domains        CommaSeparatedList   `long:"domain" env:"DOMAIN" description:"Only allow given email domains, can be set multiple times"`
 	LifetimeString int                  `long:"lifetime" env:"LIFETIME" default:"43200" description:"Lifetime in seconds"`
 	Path           string               `long:"url-path" env:"URL_PATH" default:"/_oauth" description:"Callback URL Path"`
 	SecretString   string               `long:"secret" env:"SECRET" description:"Secret used for signing (required)" json:"-"`
@@ -45,13 +45,12 @@ type Config struct {
 	Lifetime time.Duration
 
 	// Legacy
-	CookieDomainsLegacy CookieDomains      `long:"cookie-domains" env:"COOKIE_DOMAINS" description:"DEPRECATED - Use \"cookie-domain\""`
-	CookieSecretLegacy  string             `long:"cookie-secret" env:"COOKIE_SECRET" description:"DEPRECATED - Use \"secret\""  json:"-"`
-	CookieSecureLegacy  string             `long:"cookie-secure" env:"COOKIE_SECURE" description:"DEPRECATED - Use \"insecure-cookie\""`
-	DomainsLegacy       CommaSeparatedList `long:"domains" env:"DOMAINS" description:"DEPRECATED - Use \"domain\""`
-	ClientIdLegacy      string             `long:"client-id" env:"CLIENT_ID" group:"DEPs" description:"DEPRECATED - Use \"providers.google.client-id\""`
-	ClientSecretLegacy  string             `long:"client-secret" env:"CLIENT_SECRET" description:"DEPRECATED - Use \"providers.google.client-id\""  json:"-"`
-	PromptLegacy        string             `long:"prompt" env:"PROMPT" description:"DEPRECATED - Use \"providers.google.prompt\""`
+	CookieDomainsLegacy CookieDomains `long:"cookie-domains" env:"COOKIE_DOMAINS" description:"DEPRECATED - Use \"cookie-domain\""`
+	CookieSecretLegacy  string        `long:"cookie-secret" env:"COOKIE_SECRET" description:"DEPRECATED - Use \"secret\""  json:"-"`
+	CookieSecureLegacy  string        `long:"cookie-secure" env:"COOKIE_SECURE" description:"DEPRECATED - Use \"insecure-cookie\""`
+	ClientIdLegacy      string        `long:"client-id" env:"CLIENT_ID" group:"DEPs" description:"DEPRECATED - Use \"providers.google.client-id\""`
+	ClientSecretLegacy  string        `long:"client-secret" env:"CLIENT_SECRET" description:"DEPRECATED - Use \"providers.google.client-id\""  json:"-"`
+	PromptLegacy        string        `long:"prompt" env:"PROMPT" description:"DEPRECATED - Use \"providers.google.prompt\""`
 }
 
 func NewGlobalConfig() Config {
@@ -125,10 +124,6 @@ func NewConfig(args []string) (Config, error) {
 		fmt.Println("cookie-domains config option is deprecated, please use cookie-domain")
 		c.CookieDomains = append(c.CookieDomains, c.CookieDomainsLegacy...)
 	}
-	if len(c.DomainsLegacy) > 0 {
-		fmt.Println("domains config option is deprecated, please use domain")
-		c.Domains = append(c.Domains, c.DomainsLegacy...)
-	}
 
 	// Transformations
 	if len(c.Path) > 0 && c.Path[0] != '/' {

internal/config_test.go

@@ -120,7 +120,6 @@ func TestConfigParseRuleError(t *testing.T) {
 	assert.Equal(map[string]*Rule{}, c.Rules)
 }
 
-
 func TestConfigFlagBackwardsCompatability(t *testing.T) {
 	assert := assert.New(t)
 	c, err := NewConfig([]string{
@@ -132,7 +131,7 @@ func TestConfigFlagBackwardsCompatability(t *testing.T) {
 		"--cookie-secure=false",
 		"--cookie-domains=test1.com,example.org",
 		"--cookie-domain=another1.net",
-		"--domains=test2.com,example.org",
+		"--domain=test2.com,example.org",
 		"--domain=another2.net",
 		"--whitelist=test3.com,example.org",
 		"--whitelist=another3.net",
@@ -147,7 +146,7 @@ func TestConfigFlagBackwardsCompatability(t *testing.T) {
 	}
 	assert.Equal(expected1, c.CookieDomains, "should read legacy comma separated list cookie-domains")
 
-	expected2 := []string{"another2.net", "test2.com", "example.org"}
+	expected2 := CommaSeparatedList{"test2.com", "example.org", "another2.net"}
 	assert.Equal(expected2, c.Domains, "should read legacy comma separated list domains")
 
 	expected3 := CommaSeparatedList{"test3.com", "example.org", "another3.net"}
@@ -222,6 +221,70 @@ func TestConfigParseEnvironment(t *testing.T) {
 
 	assert.Equal("env_cookie_name", c.CookieName, "variable should be read from environment")
 	assert.Equal("env_client_id", c.Providers.Google.ClientId, "namespace variable should be read from environment")
+
+	os.Unsetenv("COOKIE_NAME")
+	os.Unsetenv("PROVIDERS_GOOGLE_CLIENT_ID")
+}
+
+func TestConfigParseEnvironmentBackwardsCompatability(t *testing.T) {
+	assert := assert.New(t)
+	vars := map[string]string{
+		"CLIENT_ID":      "clientid",
+		"CLIENT_SECRET":  "verysecret",
+		"PROMPT":         "prompt",
+		"COOKIE_SECRET":  "veryverysecret",
+		"LIFETIME":       "200",
+		"COOKIE_SECURE":  "false",
+		"COOKIE_DOMAINS": "test1.com,example.org",
+		"COOKIE_DOMAIN":  "another1.net",
+		"DOMAIN":         "test2.com,example.org",
+		"WHITELIST":      "test3.com,example.org",
+	}
+	for k, v := range vars {
+		os.Setenv(k, v)
+	}
+	c, err := NewConfig([]string{})
+	require.Nil(t, err)
+
+	// The following used to be passed as comma separated list
+	expected1 := []CookieDomain{
+		*NewCookieDomain("another1.net"),
+		*NewCookieDomain("test1.com"),
+		*NewCookieDomain("example.org"),
+	}
+	assert.Equal(expected1, c.CookieDomains, "should read legacy comma separated list cookie-domains")
+
+	expected2 := CommaSeparatedList{"test2.com", "example.org"}
+	assert.Equal(expected2, c.Domains, "should read legacy comma separated list domains")
+
+	expected3 := CommaSeparatedList{"test3.com", "example.org"}
+	assert.Equal(expected3, c.Whitelist, "should read legacy comma separated list whitelist")
+
+	// Name changed
+	assert.Equal([]byte("veryverysecret"), c.Secret)
+
+	// Google provider params used to be top level
+	assert.Equal("clientid", c.ClientIdLegacy)
+	assert.Equal("clientid", c.Providers.Google.ClientId, "--client-id should set providers.google.client-id")
+	assert.Equal("verysecret", c.ClientSecretLegacy)
+	assert.Equal("verysecret", c.Providers.Google.ClientSecret, "--client-secret should set providers.google.client-secret")
+	assert.Equal("prompt", c.PromptLegacy)
+	assert.Equal("prompt", c.Providers.Google.Prompt, "--prompt should set providers.google.promot")
+
+	// "cookie-secure" used to be a standard go bool flag that could take
+	// true, TRUE, 1, false, FALSE, 0 etc. values.
+	// Here we're checking that format is still suppoted
+	assert.Equal("false", c.CookieSecureLegacy)
+	assert.True(c.InsecureCookie, "--cookie-secure=false should set insecure-cookie true")
+
+	c, err = NewConfig([]string{"--cookie-secure=TRUE"})
+	assert.Nil(err)
+	assert.Equal("TRUE", c.CookieSecureLegacy)
+	assert.False(c.InsecureCookie, "--cookie-secure=TRUE should set insecure-cookie false")
+
+	for k := range vars {
+		os.Unsetenv(k)
+	}
 }
 
 func TestConfigTransformation(t *testing.T) {

internal/server.go

@@ -72,35 +72,25 @@ func (s *Server) AuthHandler(rule string) http.HandlerFunc {
 		// Get auth cookie
 		c, err := r.Cookie(config.CookieName)
 		if err != nil {
-			// Error indicates no cookie, generate nonce
-			err, nonce := Nonce()
-			if err != nil {
-				logger.Errorf("Error generating nonce, %v", err)
-				http.Error(w, "Service unavailable", 503)
-				return
-			}
-
-			// Set the CSRF cookie
-			http.SetCookie(w, MakeCSRFCookie(r, nonce))
-			logger.Debug("Set CSRF cookie and redirecting to google login")
-
-			// Forward them on
-			http.Redirect(w, r, GetLoginURL(r, nonce), http.StatusTemporaryRedirect)
-
-			logger.Debug("Done")
+			s.authRedirect(logger, w, r)
 			return
 		}
 
 		// Validate cookie
-		valid, email, err := ValidateCookie(r, c)
-		if !valid {
-			logger.Errorf("Invalid cookie: %v", err)
-			http.Error(w, "Not authorized", 401)
+		email, err := ValidateCookie(r, c)
+		if err != nil {
+			if err.Error() == "Cookie has expired" {
+				logger.Info("Cookie has expired")
+				s.authRedirect(logger, w, r)
+			} else {
+				logger.Errorf("Invalid cookie: %v", err)
+				http.Error(w, "Not authorized", 401)
+			}
 			return
 		}
 
 		// Validate user
-		valid = ValidateEmail(email)
+		valid := ValidateEmail(email)
 		if !valid {
 			logger.WithFields(logrus.Fields{
 				"email": email,
@@ -167,6 +157,26 @@ func (s *Server) AuthCallbackHandler() http.HandlerFunc {
 	}
 }
 
+func (s *Server) authRedirect(logger *logrus.Entry, w http.ResponseWriter, r *http.Request) {
+	// Error indicates no cookie, generate nonce
+	err, nonce := Nonce()
+	if err != nil {
+		logger.Errorf("Error generating nonce, %v", err)
+		http.Error(w, "Service unavailable", 503)
+		return
+	}
+
+	// Set the CSRF cookie
+	http.SetCookie(w, MakeCSRFCookie(r, nonce))
+	logger.Debug("Set CSRF cookie and redirecting to google login")
+
+	// Forward them on
+	http.Redirect(w, r, GetLoginURL(r, nonce), http.StatusTemporaryRedirect)
+
+	logger.Debug("Done")
+	return
+}
+
 func (s *Server) logger(r *http.Request, rule, msg string) *logrus.Entry {
 	// Create logger
 	logger := log.WithFields(logrus.Fields{

internal/server_test.go

@@ -8,8 +8,10 @@ import (
 	"net/url"
 	"strings"
 	"testing"
+	"time"
 
 	"github.com/stretchr/testify/assert"
+	"github.com/stretchr/testify/require"
 )
 
 // TODO:
@@ -27,7 +29,7 @@ func init() {
  * Tests
  */
 
-func TestServerAuthHandler(t *testing.T) {
+func TestServerAuthHandlerInvalid(t *testing.T) {
 	assert := assert.New(t)
 	config, _ = NewConfig([]string{})
 
@@ -57,13 +59,46 @@ func TestServerAuthHandler(t *testing.T) {
 
 	res, _ = doHttpRequest(req, c)
 	assert.Equal(401, res.StatusCode, "invalid email should not be authorised")
+}
+
+func TestServerAuthHandlerExpired(t *testing.T) {
+	assert := assert.New(t)
+	config, _ = NewConfig([]string{})
+	config.Lifetime = time.Second * time.Duration(-1)
+	config.Domains = []string{"test.com"}
+
+	// Should redirect expired cookie
+	req := newDefaultHttpRequest("/foo")
+	c := MakeCookie(req, "test@example.com")
+	res, _ := doHttpRequest(req, c)
+	assert.Equal(307, res.StatusCode, "request with expired cookie should be redirected")
+
+	// Check for CSRF cookie
+	var cookie *http.Cookie
+	for _, c := range res.Cookies() {
+		if c.Name == config.CSRFCookieName {
+			cookie = c
+		}
+	}
+	assert.NotNil(cookie)
+
+	// Check redirection location
+	fwd, _ := res.Location()
+	assert.Equal("https", fwd.Scheme, "request with expired cookie should be redirected to google")
+	assert.Equal("accounts.google.com", fwd.Host, "request with expired cookie should be redirected to google")
+	assert.Equal("/o/oauth2/auth", fwd.Path, "request with expired cookie should be redirected to google")
+}
+
+func TestServerAuthHandlerValid(t *testing.T) {
+	assert := assert.New(t)
+	config, _ = NewConfig([]string{})
 
 	// Should allow valid request email
-	req = newDefaultHttpRequest("/foo")
-	c = MakeCookie(req, "test@example.com")
+	req := newDefaultHttpRequest("/foo")
+	c := MakeCookie(req, "test@example.com")
 	config.Domains = []string{}
 
-	res, _ = doHttpRequest(req, c)
+	res, _ := doHttpRequest(req, c)
 	assert.Equal(200, res.StatusCode, "valid request should be allowed")
 
 	// Should pass through user
@@ -266,6 +301,41 @@ func TestServerRouteQuery(t *testing.T) {
 	assert.Equal(200, res.StatusCode, "request matching allow rule should be allowed")
 }
 
+func TestServerAuthCallbackWithRules(t *testing.T) {
+	assert := assert.New(t)
+	config, _ = NewConfig([]string{})
+	config.Rules = map[string]*Rule{
+		"1": {
+			Action: "auth",
+			Rule:   "Host(`example.com`) && Path(`/private`)",
+		},
+		"2": {
+			Action: "allow",
+			Rule:   "Host(`example.com`)",
+		},
+	}
+	// Should allow /test request
+	req := newHttpRequest("GET", "https://example.com/", "/test")
+	res, _ := doHttpRequest(req, nil)
+	assert.Equal(200, res.StatusCode, "request matching allow rule should be allowed")
+
+	// Should block /private request
+	req = newHttpRequest("GET", "https://example.com/", "/private")
+	res, _ = doHttpRequest(req, nil)
+	assert.Equal(307, res.StatusCode, "request matching auth rule should require auth")
+
+	// Should allow callback request
+	req = newHttpRequest("GET", "https://example.com/", "/_oauth?state=12345678901234567890123456789012:https://example.com/private")
+	c := MakeCSRFCookie(req, "12345678901234567890123456789012")
+	res, _ = doHttpRequest(req, c)
+	require.Equal(t, 307, res.StatusCode, "callback request should be redirected")
+
+	fwd, _ := res.Location()
+	assert.Equal("http", fwd.Scheme, "callback request should be correctly redirected")
+	assert.Equal("example.com", fwd.Host, "callback request should be correctly redirected")
+	assert.Equal("/private", fwd.Path, "callback request should be correctly redirected")
+}
+
 /**
  * Utilities
  */