From e9b5631eed88461bbd2c7728b244a7de9b42e640 Mon Sep 17 00:00:00 2001 From: Jehiah Czebotar Date: Mon, 22 Jun 2015 15:10:08 -0400 Subject: [PATCH] cookie refresh: validation fixes, interval changes * refresh now calculated as duration from cookie set --- contrib/oauth2_proxy.cfg.example | 21 +++++----- cookies.go | 24 +++++++---- oauthproxy.go | 28 +++++++------ oauthproxy_test.go | 70 ++++++++++++++++++++++++-------- providers/internal_util.go | 23 ++++++++--- 5 files changed, 112 insertions(+), 54 deletions(-) diff --git a/contrib/oauth2_proxy.cfg.example b/contrib/oauth2_proxy.cfg.example index 1ba1c27..4006850 100644 --- a/contrib/oauth2_proxy.cfg.example +++ b/contrib/oauth2_proxy.cfg.example @@ -54,16 +54,17 @@ # custom_templates_dir = "" ## Cookie Settings -## Name - the cookie name -## 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 -## is set -## Domain - (optional) cookie domain to force cookies to (ie: .yourcompany.com) -## Expire - (duration) expire timeframe for cookie -## Refresh - (duration) refresh the cookie when less than this much time remains before -## expiration; should be less than cookie_expire; set to 0 to disable. -## Refresh revalidated the OAuth token to ensure it is still valid. ie: 24h -## Secure - secure cookies are only sent by the browser of a HTTPS connection (recommended) +## Name - the cookie name +## 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 +## is set +## Domain - (optional) cookie domain to force cookies to (ie: .yourcompany.com) +## Expire - (duration) expire timeframe for cookie +## Refresh - (duration) refresh the cookie when duration has elapsed after cookie was initially set. +## Should be less than cookie_expire; set to 0 to disable. +## On refresh, OAuth token is re-validated. +## (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 = "" diff --git a/cookies.go b/cookies.go index 8d7ce63..511be37 100644 --- a/cookies.go +++ b/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 diff --git a/oauthproxy.go b/oauthproxy.go index 6db668f..2591384 100644 --- a/oauthproxy.go +++ b/oauthproxy.go @@ -109,8 +109,12 @@ func NewOauthProxy(opts *Options, validator func(string) bool) *OauthProxy { if domain == "" { domain = "" } + 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( - value, p.AesCipher) + email, user, access_token, err = parseCookieValue(value, p.AesCipher) } } if err != nil { log.Printf(err.Error()) ok = false - } else if p.CookieRefresh != time.Duration(0) { - expires := timestamp.Add(p.CookieExpire) - refresh_threshold := time.Now().Add(p.CookieRefresh) - if refresh_threshold.Unix() > expires.Unix() { + } else if ok && p.CookieRefresh != time.Duration(0) { + refresh := timestamp.Add(p.CookieRefresh) + if refresh.Before(time.Now()) { ok = p.Validator(email) && p.provider.ValidateToken(access_token) if ok { p.SetCookie(rw, req, value) diff --git a/oauthproxy_test.go b/oauthproxy_test.go index f53d31e..ed02b88 100644 --- a/oauthproxy_test.go +++ b/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 { - cookie_value, _ := buildCookieValue( - value, p.proxy.AesCipher, access_token) - return p.proxy.MakeCookie(p.req, cookie_value, p.opts.CookieExpire) +func (p *ProcessCookieTest) MakeCookie(value, access_token string, ref time.Time) *http.Cookie { + cookie_value, _ := buildCookieValue(value, p.proxy.AesCipher, access_token) + return p.proxy.MakeCookie(p.req, cookie_value, p.opts.CookieExpire, ref) } 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 - cookie := pc_test.MakeCookie("michael.bland@gsa.gov", "my_access_token") + pc_test.proxy.CookieExpire = time.Duration(23) * time.Hour + 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"]) diff --git a/providers/internal_util.go b/providers/internal_util.go index 0e2f39a..4ccd037 100644 --- a/providers/internal_util.go +++ b/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 }