cookie refresh: validation fixes, interval changes

* refresh now calculated as duration from cookie set

contrib/oauth2_proxy.cfg.example

@@ -54,16 +54,17 @@
 # custom_templates_dir = ""
 
 ## Cookie Settings
-## Name    - the cookie name
+## Name     - the cookie name
-## Secret - the seed string for secure cookies; should be 16, 24, or 32 bytes
+## Secret   - the seed string for secure cookies; should be 16, 24, or 32 bytes
-##          for use with an AES cipher when cookie_refresh or pass_access_token
+##            for use with an AES cipher when cookie_refresh or pass_access_token
-##          is set
+##            is set
-## Domain - (optional) cookie domain to force cookies to (ie: .yourcompany.com)
+## Domain   - (optional) cookie domain to force cookies to (ie: .yourcompany.com)
-## Expire - (duration) expire timeframe for cookie
+## Expire   - (duration) expire timeframe for cookie
-## Refresh - (duration) refresh the cookie when less than this much time remains before
+## Refresh  - (duration) refresh the cookie when duration has elapsed after cookie was initially set.
-##           expiration; should be less than cookie_expire; set to 0 to disable.
+##            Should be less than cookie_expire; set to 0 to disable.
-##           Refresh revalidated the OAuth token to ensure it is still valid. ie: 24h
+##            On refresh, OAuth token is re-validated. 
-## Secure - secure cookies are only sent by the browser of a HTTPS connection (recommended)
+##            (ie: 1h means tokens are refreshed on request 1hr+ after it was set)
+## Secure   - secure cookies are only sent by the browser of a HTTPS connection (recommended)
 ## HttpOnly - httponly cookies are not readable by javascript (recommended)
 # cookie_name = "_oauth2_proxy"
 # cookie_secret = ""

cookies.go

@@ -15,29 +15,39 @@ import (
 	"time"
 )
 
-func validateCookie(cookie *http.Cookie, seed string) (string, time.Time, bool) {
+func validateCookie(cookie *http.Cookie, seed string, expiration time.Duration) (value string, t time.Time, ok bool) {
 	// value, timestamp, sig
 	parts := strings.Split(cookie.Value, "|")
 	if len(parts) != 3 {
-		return "", time.Unix(0, 0), false
+		return
 	}
 	sig := cookieSignature(seed, cookie.Name, parts[0], parts[1])
 	if checkHmac(parts[2], sig) {
 		ts, err := strconv.Atoi(parts[1])
-		if err == nil && int64(ts) > time.Now().Add(time.Duration(24)*7*time.Hour*-1).Unix() {
+		if err != nil {
+			return
+		}
+		// The expiration timestamp set when the cookie was created
+		// isn't sent back by the browser. Hence, we check whether the
+		// creation timestamp stored in the cookie falls within the
+		// window defined by (Now()-expiration, Now()].
+		t = time.Unix(int64(ts), 0)
+		if t.After(time.Now().Add(expiration*-1)) && t.Before(time.Now().Add(time.Minute*5)) {
 			// it's a valid cookie. now get the contents
 			rawValue, err := base64.URLEncoding.DecodeString(parts[0])
 			if err == nil {
-				return string(rawValue), time.Unix(int64(ts), 0), true
+				value = string(rawValue)
+				ok = true
+				return
 			}
 		}
 	}
-	return "", time.Unix(0, 0), false
+	return
 }
 
-func signedCookieValue(seed string, key string, value string) string {
+func signedCookieValue(seed string, key string, value string, now time.Time) string {
 	encodedValue := base64.URLEncoding.EncodeToString([]byte(value))
-	timeStr := fmt.Sprintf("%d", time.Now().Unix())
+	timeStr := fmt.Sprintf("%d", now.Unix())
 	sig := cookieSignature(seed, key, encodedValue, timeStr)
 	cookieVal := fmt.Sprintf("%s|%s|%s", encodedValue, timeStr, sig)
 	return cookieVal

oauthproxy.go

@@ -109,8 +109,12 @@ func NewOauthProxy(opts *Options, validator func(string) bool) *OauthProxy {
 	if domain == "" {
 		domain = "<default>"
 	}
+	refresh := "disabled"
+	if opts.CookieRefresh != time.Duration(0) {
+		refresh = fmt.Sprintf("after %s", opts.CookieRefresh)
+	}
 
-	log.Printf("Cookie settings: name:%s secure(https):%v httponly:%v expiry:%s domain:%s", opts.CookieName, opts.CookieSecure, opts.CookieHttpOnly, opts.CookieExpire, domain)
+	log.Printf("Cookie settings: name:%s secure(https):%v httponly:%v expiry:%s domain:%s refresh:%s", opts.CookieName, opts.CookieSecure, opts.CookieHttpOnly, opts.CookieExpire, domain, refresh)
 
 	var aes_cipher cipher.Block
 	if opts.PassAccessToken || (opts.CookieRefresh != time.Duration(0)) {
@@ -191,7 +195,7 @@ func (p *OauthProxy) redeemCode(host, code string) (string, string, error) {
 	return access_token, email, nil
 }
 
-func (p *OauthProxy) MakeCookie(req *http.Request, value string, expiration time.Duration) *http.Cookie {
+func (p *OauthProxy) MakeCookie(req *http.Request, value string, expiration time.Duration, now time.Time) *http.Cookie {
 	domain := req.Host
 	if h, _, err := net.SplitHostPort(domain); err == nil {
 		domain = h
@@ -204,7 +208,7 @@ func (p *OauthProxy) MakeCookie(req *http.Request, value string, expiration time
 	}
 
 	if value != "" {
-		value = signedCookieValue(p.CookieSeed, p.CookieName, value)
+		value = signedCookieValue(p.CookieSeed, p.CookieName, value, now)
 	}
 
 	return &http.Cookie{
@@ -214,16 +218,16 @@ func (p *OauthProxy) MakeCookie(req *http.Request, value string, expiration time
 		Domain:   domain,
 		HttpOnly: p.CookieHttpOnly,
 		Secure:   p.CookieSecure,
-		Expires:  time.Now().Add(expiration),
+		Expires:  now.Add(expiration),
 	}
 }
 
 func (p *OauthProxy) ClearCookie(rw http.ResponseWriter, req *http.Request) {
-	http.SetCookie(rw, p.MakeCookie(req, "", time.Duration(1)*time.Hour*-1))
+	http.SetCookie(rw, p.MakeCookie(req, "", time.Hour*-1, time.Now()))
 }
 
 func (p *OauthProxy) SetCookie(rw http.ResponseWriter, req *http.Request, val string) {
-	http.SetCookie(rw, p.MakeCookie(req, val, p.CookieExpire))
+	http.SetCookie(rw, p.MakeCookie(req, val, p.CookieExpire, time.Now()))
 }
 
 func (p *OauthProxy) ProcessCookie(rw http.ResponseWriter, req *http.Request) (email, user, access_token string, ok bool) {
@@ -231,19 +235,17 @@ func (p *OauthProxy) ProcessCookie(rw http.ResponseWriter, req *http.Request) (e
 	var timestamp time.Time
 	cookie, err := req.Cookie(p.CookieName)
 	if err == nil {
-		value, timestamp, ok = validateCookie(cookie, p.CookieSeed)
+		value, timestamp, ok = validateCookie(cookie, p.CookieSeed, p.CookieExpire)
 		if ok {
-			email, user, access_token, err = parseCookieValue(
+			email, user, access_token, err = parseCookieValue(value, p.AesCipher)
-				value, p.AesCipher)
 		}
 	}
 	if err != nil {
 		log.Printf(err.Error())
 		ok = false
-	} else if p.CookieRefresh != time.Duration(0) {
+	} else if ok && p.CookieRefresh != time.Duration(0) {
-		expires := timestamp.Add(p.CookieExpire)
+		refresh := timestamp.Add(p.CookieRefresh)
-		refresh_threshold := time.Now().Add(p.CookieRefresh)
+		if refresh.Before(time.Now()) {
-		if refresh_threshold.Unix() > expires.Unix() {
 			ok = p.Validator(email) && p.provider.ValidateToken(access_token)
 			if ok {
 				p.SetCookie(rw, req, value)

oauthproxy_test.go

@@ -348,13 +348,12 @@ func NewProcessCookieTest(opts ProcessCookieTestOpts) *ProcessCookieTest {
 
 	pc_test.opts = NewOptions()
 	pc_test.opts.Upstreams = append(pc_test.opts.Upstreams, "unused")
-	pc_test.opts.CookieSecret = "foobar"
 	pc_test.opts.ClientID = "bazquux"
 	pc_test.opts.ClientSecret = "xyzzyplugh"
 	pc_test.opts.CookieSecret = "0123456789abcdef"
 	// First, set the CookieRefresh option so proxy.AesCipher is created,
 	// needed to encrypt the access_token.
-	pc_test.opts.CookieRefresh = time.Duration(24) * time.Hour
+	pc_test.opts.CookieRefresh = time.Hour
 	pc_test.opts.Validate()
 
 	pc_test.proxy = NewOauthProxy(pc_test.opts, func(email string) bool {
@@ -379,14 +378,13 @@ func NewProcessCookieTestWithDefaults() *ProcessCookieTest {
 	})
 }
 
-func (p *ProcessCookieTest) MakeCookie(value, access_token string) *http.Cookie {
+func (p *ProcessCookieTest) MakeCookie(value, access_token string, ref time.Time) *http.Cookie {
-	cookie_value, _ := buildCookieValue(
+	cookie_value, _ := buildCookieValue(value, p.proxy.AesCipher, access_token)
-		value, p.proxy.AesCipher, access_token)
+	return p.proxy.MakeCookie(p.req, cookie_value, p.opts.CookieExpire, ref)
-	return p.proxy.MakeCookie(p.req, cookie_value, p.opts.CookieExpire)
 }
 
 func (p *ProcessCookieTest) AddCookie(value, access_token string) {
-	p.req.AddCookie(p.MakeCookie(value, access_token))
+	p.req.AddCookie(p.MakeCookie(value, access_token, time.Now()))
 }
 
 func (p *ProcessCookieTest) ProcessCookie() (email, user, access_token string, ok bool) {
@@ -416,7 +414,7 @@ func TestProcessCookieFailIfParsingCookieValueFails(t *testing.T) {
 		pc_test.proxy.AesCipher, "my_access_token")
 	pc_test.req.AddCookie(pc_test.proxy.MakeCookie(
 		pc_test.req, value+"some bogus bytes",
-		pc_test.opts.CookieExpire))
+		pc_test.opts.CookieExpire, time.Now()))
 	_, _, _, ok := pc_test.ProcessCookie()
 	assert.Equal(t, false, ok)
 }
@@ -424,7 +422,8 @@ func TestProcessCookieFailIfParsingCookieValueFails(t *testing.T) {
 func TestProcessCookieRefreshNotSet(t *testing.T) {
 	pc_test := NewProcessCookieTestWithDefaults()
 	pc_test.proxy.CookieExpire = time.Duration(23) * time.Hour
-	cookie := pc_test.MakeCookie("michael.bland@gsa.gov", "")
+	reference := time.Now().Add(time.Duration(-2) * time.Hour)
+	cookie := pc_test.MakeCookie("michael.bland@gsa.gov", "", reference)
 	pc_test.req.AddCookie(cookie)
 
 	_, _, _, ok := pc_test.ProcessCookie()
@@ -435,10 +434,11 @@ func TestProcessCookieRefreshNotSet(t *testing.T) {
 func TestProcessCookieRefresh(t *testing.T) {
 	pc_test := NewProcessCookieTestWithDefaults()
 	pc_test.proxy.CookieExpire = time.Duration(23) * time.Hour
-	cookie := pc_test.MakeCookie("michael.bland@gsa.gov", "my_access_token")
+	reference := time.Now().Add(time.Duration(-2) * time.Hour)
+	cookie := pc_test.MakeCookie("michael.bland@gsa.gov", "my_access_token", reference)
 	pc_test.req.AddCookie(cookie)
 
-	pc_test.proxy.CookieRefresh = time.Duration(24) * time.Hour
+	pc_test.proxy.CookieRefresh = time.Hour
 	_, _, _, ok := pc_test.ProcessCookie()
 	assert.Equal(t, true, ok)
 	assert.NotEqual(t, []string(nil), pc_test.rw.HeaderMap["Set-Cookie"])
@@ -446,25 +446,58 @@ func TestProcessCookieRefresh(t *testing.T) {
 
 func TestProcessCookieRefreshThresholdNotCrossed(t *testing.T) {
 	pc_test := NewProcessCookieTestWithDefaults()
-	pc_test.proxy.CookieExpire = time.Duration(25) * time.Hour
+	pc_test.proxy.CookieExpire = time.Duration(23) * time.Hour
-	cookie := pc_test.MakeCookie("michael.bland@gsa.gov", "my_access_token")
+	reference := time.Now().Add(time.Duration(-30) * time.Minute)
+	cookie := pc_test.MakeCookie("michael.bland@gsa.gov", "my_access_token", reference)
 	pc_test.req.AddCookie(cookie)
 
-	pc_test.proxy.CookieRefresh = time.Duration(24) * time.Hour
+	pc_test.proxy.CookieRefresh = time.Hour
 	_, _, _, ok := pc_test.ProcessCookie()
 	assert.Equal(t, true, ok)
 	assert.Equal(t, []string(nil), pc_test.rw.HeaderMap["Set-Cookie"])
 }
 
+func TestProcessCookieFailIfCookieExpired(t *testing.T) {
+	pc_test := NewProcessCookieTestWithDefaults()
+	pc_test.proxy.CookieExpire = time.Duration(24) * time.Hour
+	reference := time.Now().Add(time.Duration(25) * time.Hour * -1)
+	cookie := pc_test.MakeCookie("michael.bland@gsa.gov", "my_access_token", reference)
+	pc_test.req.AddCookie(cookie)
+
+	if _, _, _, ok := pc_test.ProcessCookie(); ok {
+		t.Error("ProcessCookie() should have failed")
+	}
+	if set_cookie := pc_test.rw.HeaderMap["Set-Cookie"]; set_cookie != nil {
+		t.Error("expected Set-Cookie to be nil, instead was: ", set_cookie)
+	}
+}
+
+func TestProcessCookieFailIfRefreshSetAndCookieExpired(t *testing.T) {
+	pc_test := NewProcessCookieTestWithDefaults()
+	pc_test.proxy.CookieExpire = time.Duration(24) * time.Hour
+	reference := time.Now().Add(time.Duration(25) * time.Hour * -1)
+	cookie := pc_test.MakeCookie("michael.bland@gsa.gov", "my_access_token", reference)
+	pc_test.req.AddCookie(cookie)
+
+	pc_test.proxy.CookieRefresh = time.Hour
+	if _, _, _, ok := pc_test.ProcessCookie(); ok {
+		t.Error("ProcessCookie() should have failed")
+	}
+	if set_cookie := pc_test.rw.HeaderMap["Set-Cookie"]; set_cookie != nil {
+		t.Error("expected Set-Cookie to be nil, instead was: ", set_cookie)
+	}
+}
+
 func TestProcessCookieFailIfRefreshSetAndTokenNoLongerValid(t *testing.T) {
 	pc_test := NewProcessCookieTest(ProcessCookieTestOpts{
 		provider_validate_cookie_response: false,
 	})
 	pc_test.proxy.CookieExpire = time.Duration(23) * time.Hour
-	cookie := pc_test.MakeCookie("michael.bland@gsa.gov", "my_access_token")
+	reference := time.Now().Add(time.Duration(-24) * time.Hour)
+	cookie := pc_test.MakeCookie("michael.bland@gsa.gov", "my_access_token", reference)
 	pc_test.req.AddCookie(cookie)
 
-	pc_test.proxy.CookieRefresh = time.Duration(24) * time.Hour
+	pc_test.proxy.CookieRefresh = time.Hour
 	_, _, _, ok := pc_test.ProcessCookie()
 	assert.Equal(t, false, ok)
 	assert.Equal(t, []string(nil), pc_test.rw.HeaderMap["Set-Cookie"])
@@ -475,10 +508,11 @@ func TestProcessCookieFailIfRefreshSetAndUserNoLongerValid(t *testing.T) {
 	pc_test.validate_user = false
 
 	pc_test.proxy.CookieExpire = time.Duration(23) * time.Hour
-	cookie := pc_test.MakeCookie("michael.bland@gsa.gov", "my_access_token")
+	reference := time.Now().Add(time.Duration(-2) * time.Hour)
+	cookie := pc_test.MakeCookie("michael.bland@gsa.gov", "my_access_token", reference)
 	pc_test.req.AddCookie(cookie)
 
-	pc_test.proxy.CookieRefresh = time.Duration(24) * time.Hour
+	pc_test.proxy.CookieRefresh = time.Hour
 	_, _, _, ok := pc_test.ProcessCookie()
 	assert.Equal(t, false, ok)
 	assert.Equal(t, []string(nil), pc_test.rw.HeaderMap["Set-Cookie"])

providers/internal_util.go

@@ -1,23 +1,34 @@
 package providers
 
 import (
-	"github.com/bitly/oauth2_proxy/api"
+	"io/ioutil"
 	"log"
 	"net/http"
+	"net/url"
+
+	"github.com/bitly/oauth2_proxy/api"
 )
 
 func validateToken(p Provider, access_token string, header http.Header) bool {
 	if access_token == "" || p.Data().ValidateUrl == nil {
 		return false
 	}
-	url := p.Data().ValidateUrl.String()
+	endpoint := p.Data().ValidateUrl.String()
 	if len(header) == 0 {
-		url = url + "?access_token=" + access_token
+		params := url.Values{"access_token": {access_token}}
+		endpoint = endpoint + "?" + params.Encode()
 	}
-	if resp, err := api.RequestUnparsedResponse(url, header); err != nil {
+	resp, err := api.RequestUnparsedResponse(endpoint, header)
+	if err != nil {
 		log.Printf("token validation request failed: %s", err)
 		return false
-	} else {
-		return resp.StatusCode == 200
 	}
+
+	body, _ := ioutil.ReadAll(resp.Body)
+	resp.Body.Close()
+	if resp.StatusCode == 200 {
+		return true
+	}
+	log.Printf("token validation request failed: status %d - %s", resp.StatusCode, body)
+	return false
 }