Fix JWTs for auth-durations less than 5 mins (#1318)

* WIP

* Fix JWTs for auth-durations less than 5 mins

For auth-duration = 0 the JWT now understands that there does not
need to be duration checks.

For auth-duration < 5 minutes > 0 the JWT lifespan will be 1/2
of auth-duration to allow one extension

There is likely a range of very short auth-duration times like, say,
less than 5 seconds that would never allow a person to login simply
because the time of issue and request is longer.

* Update changelog

CHANGELOG.md

@@ -4,6 +4,7 @@
   1. [#1257](https://github.com/influxdata/chronograf/issues/1257): Fix function selection in query builder
   1. [#1244](https://github.com/influxdata/chronograf/pull/1244): Fix env var name for Google client secret
   1. [#1269](https://github.com/influxdata/chronograf/issues/1269): Add more functionality to query config generation
+  1. [#1318](https://github.com/influxdata/chronograf/issues/1318): Fix JWT refresh for auth-duration zero and less than 5 min
 
 ### Features
   1. [#1232](https://github.com/influxdata/chronograf/pull/1232): Fuse the query builder and raw query editor

oauth2/cookies.go

@@ -26,10 +26,18 @@ type cookie struct {
 
 // NewCookieJWT creates an Authenticator that uses cookies for auth
 func NewCookieJWT(secret string, lifespan time.Duration) Authenticator {
+	inactivity := DefaultInactivityDuration
+	// Server interprets a token duration longer than the cookie lifespan as
+	// a token that was issued by a server with a longer auth-duration and is
+	// thus invalid, as a security precaution. So, inactivity must be set to
+	// be less than lifespan.
+	if lifespan > 0 && inactivity > lifespan {
+		inactivity = lifespan / 2 // half of the lifespan ensures tokens can be refreshed once.
+	}
 	return &cookie{
 		Name:       DefaultCookieName,
 		Lifespan:   lifespan,
-		Inactivity: DefaultInactivityDuration,
+		Inactivity: inactivity,
 		Now:        DefaultNowTime,
 		Tokens: &JWT{
 			Secret: secret,
@@ -44,6 +52,7 @@ func (c *cookie) Validate(ctx context.Context, r *http.Request) (Principal, erro
 	if err != nil {
 		return Principal{}, ErrAuthentication
 	}
+
 	return c.Tokens.ValidPrincipal(ctx, Token(cookie.Value), c.Lifespan)
 }
 
@@ -105,15 +114,22 @@ func (c *cookie) setCookie(w http.ResponseWriter, value string, exp time.Time) {
 
 	// Only set a cookie to be persistent (endure beyond the browser session)
 	// if auth duration is greater than zero
-	if c.Lifespan > 0 || exp.Before(c.Now()) {
+	if c.Lifespan > 0 {
 		cookie.Expires = exp
 	}
-
 	http.SetCookie(w, &cookie)
 }
 
 // Expire returns a cookie that will expire an existing cookie
 func (c *cookie) Expire(w http.ResponseWriter) {
 	// to expire cookie set the time in the past
-	c.setCookie(w, "none", c.Now().Add(-1*time.Hour))
+	cookie := http.Cookie{
+		Name:     DefaultCookieName,
+		Value:    "none",
+		HttpOnly: true,
+		Path:     "/",
+		Expires:  c.Now().Add(-1 * time.Hour),
+	}
+
+	http.SetCookie(w, &cookie)
 }

oauth2/cookies_test.go

@@ -152,9 +152,25 @@ func TestCookieValidate(t *testing.T) {
 }
 
 func TestNewCookieJWT(t *testing.T) {
-	auth := NewCookieJWT("secret", time.Second)
-	if _, ok := auth.(*cookie); !ok {
+	auth := NewCookieJWT("secret", 2*time.Second)
+	if cookie, ok := auth.(*cookie); !ok {
 		t.Errorf("NewCookieJWT() did not create cookie Authenticator")
+	} else if cookie.Inactivity != time.Second {
+		t.Errorf("NewCookieJWT() inactivity was not two seconds: %s", cookie.Inactivity)
+	}
+
+	auth = NewCookieJWT("secret", time.Hour)
+	if cookie, ok := auth.(*cookie); !ok {
+		t.Errorf("NewCookieJWT() did not create cookie Authenticator")
+	} else if cookie.Inactivity != DefaultInactivityDuration {
+		t.Errorf("NewCookieJWT() inactivity was not five minutes: %s", cookie.Inactivity)
+	}
+
+	auth = NewCookieJWT("secret", 0)
+	if cookie, ok := auth.(*cookie); !ok {
+		t.Errorf("NewCookieJWT() did not create cookie Authenticator")
+	} else if cookie.Inactivity != DefaultInactivityDuration {
+		t.Errorf("NewCookieJWT() inactivity was not five minutes: %s", cookie.Inactivity)
 	}
 }
 

oauth2/jwt.go

@@ -45,7 +45,7 @@ func (c *Claims) Valid() error {
 }
 
 // ValidPrincipal checks if the jwtToken is signed correctly and validates with Claims.  lifespan is the
-// maximum valid lifetime of a token.
+// maximum valid lifetime of a token.  If the lifespan is 0 then the auth lifespan duration is not checked.
 func (j *JWT) ValidPrincipal(ctx context.Context, jwtToken Token, lifespan time.Duration) (Principal, error) {
 	gojwt.TimeFunc = j.Now
 
@@ -86,8 +86,9 @@ func (j *JWT) ValidClaims(jwtToken Token, lifespan time.Duration, alg gojwt.Keyf
 
 	// If the duration of the claim is longer than the auth lifespan then this is
 	// an invalid claim because server assumes that lifespan is the maximum possible
-	// duration
-	if exp.Sub(iat) > lifespan {
+	// duration.  However, a lifespan of zero means that the duration comparison
+	// against the auth duration is not needed.
+	if lifespan > 0 && exp.Sub(iat) > lifespan {
 		return Principal{}, fmt.Errorf("claims duration is different from auth lifespan")
 	}
 

oauth2/jwt_test.go

@@ -79,14 +79,14 @@ func TestAuthenticate(t *testing.T) {
 		{
 			Desc:     "Test jwt duration matches auth duration",
 			Secret:   "secret",
-			Token:    "eyJhbGciOiJIUzI1NiIsInR5cCI6IkpXVCJ9.eyJzdWIiOiIvY2hyb25vZ3JhZi92MS91c2Vycy8xIiwibmFtZSI6IkRvYyBCcm93biIsImlhdCI6LTQ0Njc3NDQwMCwiZXhwIjotNDQ2Nzc0NDAwLCJuYmYiOi00NDY3NzQ0MDB9._rZ4gOIei9PizHOABH6kLcJTA3jm8ls0YnDxtz1qeUI",
-			Duration: 500 * time.Hour,
+			Token:    "eyJhbGciOiJIUzI1NiIsInR5cCI6IkpXVCJ9.eyJleHAiOi00NDY3NzQzMDAsImlhdCI6LTQ0Njc3NDQwMCwiaXNzIjoiaGlsbHZhbGxleSIsIm5iZiI6LTQ0Njc3NDQwMCwic3ViIjoibWFydHlAcGluaGVhZC5uZXQifQ.njEjstpuIDnghSR7VyPPB9QlvJ6Q5JpR3ZEZ_8vGYfA",
+			Duration: time.Second,
 			Principal: oauth2.Principal{
-				Subject:   "/chronograf/v1/users/1",
+				Subject:   "marty@pinhead.net",
 				ExpiresAt: history,
-				IssuedAt:  history,
+				IssuedAt:  history.Add(100 * time.Second),
 			},
-			Err: errors.New("claims duration is different from auth duration"),
+			Err: errors.New("claims duration is different from auth lifespan"),
 		},
 	}
 	for _, test := range tests {
@@ -97,6 +97,9 @@ func TestAuthenticate(t *testing.T) {
 			},
 		}
 		principal, err := j.ValidPrincipal(context.Background(), test.Token, test.Duration)
+		if test.Err != nil && err == nil {
+			t.Fatalf("Expected err %s", test.Err.Error())
+		}
 		if err != nil {
 			if test.Err == nil {
 				t.Errorf("Error in test %s authenticating with bad token: %v", test.Desc, err)