Better support for OAuth 2.0 servers that don't return expiration times

RFC 6749 states that the "expires_in" field is optional. It's valid for OAuth 2.0 servers to return token responses without specifying an expiration time. We could add a configuration option to allow specifying a default value in case the server doesn't report one. But instead of doing that, let's use an exponential backoff.
buildbarn · Sep 2, 2023 · a9ac0c7 · a9ac0c7
1 parent 0a74138
commit a9ac0c7
Show file tree

Hide file tree

Showing 6 changed files with 162 additions and 19 deletions.
diff --git a/pkg/http/BUILD.bazel b/pkg/http/BUILD.bazel
@@ -33,6 +33,7 @@ go_library(
         "@org_golang_google_grpc//codes",
         "@org_golang_google_grpc//status",
         "@org_golang_google_protobuf//proto",
+        "@org_golang_google_protobuf//types/known/durationpb",
         "@org_golang_google_protobuf//types/known/timestamppb",
         "@org_golang_x_oauth2//:oauth2",
     ],
@@ -58,6 +59,7 @@ go_test(
         "@org_golang_google_grpc//codes",
         "@org_golang_google_grpc//status",
         "@org_golang_google_protobuf//proto",
+        "@org_golang_google_protobuf//types/known/durationpb",
         "@org_golang_google_protobuf//types/known/structpb",
         "@org_golang_google_protobuf//types/known/timestamppb",
         "@org_golang_x_oauth2//:oauth2",

diff --git a/pkg/http/oidc_authenticator.go b/pkg/http/oidc_authenticator.go
@@ -22,6 +22,7 @@ import (
 	"google.golang.org/grpc/codes"
 	"google.golang.org/grpc/status"
 	"google.golang.org/protobuf/proto"
+	"google.golang.org/protobuf/types/known/durationpb"
 	"google.golang.org/protobuf/types/known/timestamppb"
 )
 
@@ -121,7 +122,7 @@ func (a *oidcAuthenticator) setCookieValue(w http.ResponseWriter, cookieValue *o
 	return nil
 }
 
-func (a *oidcAuthenticator) getClaimsAndSetCookie(ctx context.Context, token *oauth2.Token, w http.ResponseWriter) (*auth.AuthenticationMetadata, error) {
+func (a *oidcAuthenticator) getClaimsAndSetCookie(ctx context.Context, token *oauth2.Token, defaultExpiration time.Duration, w http.ResponseWriter) (*auth.AuthenticationMetadata, error) {
 	// Obtain claims from the user info endpoint.
 	claimsRequest, err := http.NewRequestWithContext(ctx, http.MethodGet, a.userInfoURL, nil)
 	if err != nil {
@@ -157,14 +158,25 @@ func (a *oidcAuthenticator) getClaimsAndSetCookie(ctx context.Context, token *oa
 		return nil, util.StatusWrap(err, "Failed to create authentication metadata")
 	}
 
+	expiration := token.Expiry
+	if expiration.IsZero() {
+		// The access token response did not contain an
+		// expiration duration. Simply assume an expiration
+		// duration of 1 minute, growing exponentially as long
+		// as refreshing succeeds.
+		expiration = a.clock.Now().Add(defaultExpiration)
+		defaultExpiration *= 2
+	}
+
 	// Redirect to the originally requested path, with the
 	// authentication metadata stored in a cookie.
 	if err := a.setCookieValue(w, &oidc.CookieValue{
 		SessionState: &oidc.CookieValue_Authenticated_{
 			Authenticated: &oidc.CookieValue_Authenticated{
 				AuthenticationMetadata: authenticationMetadata.GetFullProto(),
-				Expiration:             timestamppb.New(token.Expiry),
+				Expiration:             timestamppb.New(expiration),
 				RefreshToken:           token.RefreshToken,
+				DefaultExpiration:      durationpb.New(defaultExpiration),
 			},
 		},
 	}); err != nil {
@@ -207,7 +219,7 @@ func (a *oidcAuthenticator) Authenticate(w http.ResponseWriter, r *http.Request)
 		}
 
 		// Redirect back to the originally requested page.
-		if _, err := a.getClaimsAndSetCookie(ctx, token, w); err != nil {
+		if _, err := a.getClaimsAndSetCookie(ctx, token, time.Minute, w); err != nil {
 			return nil, err
 		}
 		http.Redirect(w, r, authenticating.OriginalRequestUri, http.StatusSeeOther)
@@ -235,7 +247,7 @@ func (a *oidcAuthenticator) Authenticate(w http.ResponseWriter, r *http.Request)
 					Expiry:       time.Unix(0, 0),
 				},
 			).Token(); err == nil {
-				if authenticationMetadata, err := a.getClaimsAndSetCookie(ctx, refreshedToken, w); err == nil {
+				if authenticationMetadata, err := a.getClaimsAndSetCookie(ctx, refreshedToken, authenticated.DefaultExpiration.AsDuration(), w); err == nil {
 					return authenticationMetadata, nil
 				}
 			}

diff --git a/pkg/http/oidc_authenticator_test.go b/pkg/http/oidc_authenticator_test.go
@@ -23,6 +23,7 @@ import (
 	"google.golang.org/grpc/codes"
 	"google.golang.org/grpc/status"
 	"google.golang.org/protobuf/proto"
+	"google.golang.org/protobuf/types/known/durationpb"
 	"google.golang.org/protobuf/types/known/structpb"
 	"google.golang.org/protobuf/types/known/timestamppb"
 )
@@ -131,6 +132,7 @@ func TestOIDCAuthenticator(t *testing.T) {
 					Authenticated: &oidc.CookieValue_Authenticated{
 						AuthenticationMetadata: &auth.AuthenticationMetadata{},
 						Expiration:             &timestamppb.Timestamp{Seconds: 1693145873},
+						DefaultExpiration:      &durationpb.Duration{Seconds: 60},
 					},
 				},
 			})...), nil
@@ -175,7 +177,7 @@ func TestOIDCAuthenticator(t *testing.T) {
 		}, w.HeaderMap)
 	})
 
-	t.Run("RegularRequestExpiredAccessTokenWithRefreshToken", func(t *testing.T) {
+	t.Run("RegularRequestExpiredAccessTokenWithRefreshTokenWithExpiresIn", func(t *testing.T) {
 		// If the access token is expired and a refresh token is
 		// available, we may be able to continue the session
 		// without sending the user through the authorization
@@ -193,6 +195,7 @@ func TestOIDCAuthenticator(t *testing.T) {
 						AuthenticationMetadata: &auth.AuthenticationMetadata{},
 						Expiration:             &timestamppb.Timestamp{Seconds: 1693147158},
 						RefreshToken:           "RefreshToken1",
+						DefaultExpiration:      &durationpb.Duration{Seconds: 60},
 					},
 				},
 			})...), nil
@@ -252,6 +255,7 @@ func TestOIDCAuthenticator(t *testing.T) {
 						AuthenticationMetadata: expectedAuthenticationMetadata,
 						Expiration:             &timestamppb.Timestamp{Seconds: 1693150813},
 						RefreshToken:           "RefreshToken2",
+						DefaultExpiration:      &durationpb.Duration{Seconds: 60},
 					},
 				},
 			}),
@@ -276,6 +280,108 @@ func TestOIDCAuthenticator(t *testing.T) {
 		}, w.HeaderMap)
 	})
 
+	t.Run("RegularRequestExpiredAccessTokenWithRefreshTokenWithoutExpiresIn", func(t *testing.T) {
+		// It is only recommended that the access token response
+		// contains an 'expires_in' value. If it does not, let's
+		// pick an exponentially growing expiration time.
+		//
+		// More details: RFC 6749, section 4.2.2.
+		cookieAEAD.EXPECT().Open(
+			gomock.Any(),
+			[]byte{0x84, 0x4b, 0x47, 0xdd},
+			[]byte{0xac, 0x4f, 0xc2, 0x0d, 0x5b, 0x01, 0x78, 0x67},
+			nil,
+		).DoAndReturn(func(dst, nonce, ciphertext, additionalData []byte) ([]byte, error) {
+			return append(dst, protoMustMarshal(&oidc.CookieValue{
+				SessionState: &oidc.CookieValue_Authenticated_{
+					Authenticated: &oidc.CookieValue_Authenticated{
+						AuthenticationMetadata: &auth.AuthenticationMetadata{},
+						Expiration:             &timestamppb.Timestamp{Seconds: 1693631281},
+						RefreshToken:           "RefreshToken1",
+						DefaultExpiration:      &durationpb.Duration{Seconds: 60},
+					},
+				},
+			})...), nil
+		})
+		clock.EXPECT().Now().Return(time.Unix(1693631288, 0))
+		roundTripper.EXPECT().RoundTrip(gomock.Any()).DoAndReturn(func(r *http.Request) (*http.Response, error) {
+			require.Equal(t, "https://login.com/token", r.URL.String())
+			r.ParseForm()
+			require.Equal(t, url.Values{
+				"client_id":     []string{"MyClientID"},
+				"client_secret": []string{"MyClientSecret"},
+				"grant_type":    []string{"refresh_token"},
+				"refresh_token": []string{"RefreshToken1"},
+			}, r.Form)
+			return &http.Response{
+				Status:     "200 OK",
+				StatusCode: 200,
+				Body: io.NopCloser(bytes.NewBufferString(`{
+					"access_token": "AccessToken2",
+					"refresh_token": "RefreshToken2",
+					"token_type": "Bearer"
+				}`)),
+			}, nil
+		})
+		clock.EXPECT().Now().Return(time.Unix(1693631289, 0))
+		roundTripper.EXPECT().RoundTrip(gomock.Any()).DoAndReturn(func(r *http.Request) (*http.Response, error) {
+			require.Equal(t, "https://login.com/userinfo", r.URL.String())
+			require.Equal(t, http.Header{
+				"Authorization": []string{"Bearer AccessToken2"},
+			}, r.Header)
+			return &http.Response{
+				Status:     "200 OK",
+				StatusCode: 200,
+				Body: io.NopCloser(bytes.NewBufferString(`{
+					"email": "john@myserver.com",
+					"name": "John Doe"
+				}`)),
+			}, nil
+		})
+		nonce := []byte{0xa9, 0xe3, 0x9f, 0xc5}
+		expectRead(randomNumberGenerator, nonce)
+		expectedAuthenticationMetadata := &auth.AuthenticationMetadata{
+			Public: structpb.NewStructValue(&structpb.Struct{
+				Fields: map[string]*structpb.Value{
+					"email": structpb.NewStringValue("john@myserver.com"),
+					"name":  structpb.NewStringValue("John Doe"),
+				},
+			}),
+		}
+		cookieAEAD.EXPECT().Seal(
+			gomock.Any(),
+			nonce,
+			protoMustMarshal(&oidc.CookieValue{
+				SessionState: &oidc.CookieValue_Authenticated_{
+					Authenticated: &oidc.CookieValue_Authenticated{
+						AuthenticationMetadata: expectedAuthenticationMetadata,
+						Expiration:             &timestamppb.Timestamp{Seconds: 1693631349},
+						RefreshToken:           "RefreshToken2",
+						DefaultExpiration:      &durationpb.Duration{Seconds: 120},
+					},
+				},
+			}),
+			nil,
+		).DoAndReturn(func(dst, nonce, plaintext, additionalData []byte) []byte {
+			return append(dst, 0xfc, 0x39, 0x91, 0xcd, 0x66, 0xd1, 0xbb, 0x95)
+		})
+
+		w := httptest.NewRecorder()
+		r, err := http.NewRequestWithContext(ctx, http.MethodGet, "http://myserver.com/hello.png", nil)
+		r.AddCookie(&http.Cookie{
+			Name:  "CookieName",
+			Value: "hEtH3axPwg1bAXhn",
+		})
+		require.NoError(t, err)
+		metadata, err := authenticator.Authenticate(w, r)
+		require.NoError(t, err)
+		testutil.RequireEqualProto(t, expectedAuthenticationMetadata, metadata.GetFullProto())
+
+		require.Equal(t, http.Header{
+			"Set-Cookie": []string{"CookieName=qeOfxfw5kc1m0buV; Path=/; HttpOnly; Secure; SameSite=Strict"},
+		}, w.HeaderMap)
+	})
+
 	t.Run("CallbackRequestWithoutCookie", func(t *testing.T) {
 		// After authorization has been performed, the user is
 		// sent to the redirect URL. We can only finalize the

diff --git a/pkg/proto/http/oidc/BUILD.bazel b/pkg/proto/http/oidc/BUILD.bazel
@@ -8,6 +8,7 @@ proto_library(
     visibility = ["//visibility:public"],
     deps = [
         "//pkg/proto/auth:auth_proto",
+        "@com_google_protobuf//:duration_proto",
         "@com_google_protobuf//:timestamp_proto",
     ],
 )

diff --git a/pkg/proto/http/oidc/oidc.pb.go b/pkg/proto/http/oidc/oidc.pb.go
diff --git a/pkg/proto/http/oidc/oidc.proto b/pkg/proto/http/oidc/oidc.proto
@@ -2,6 +2,7 @@ syntax = "proto3";
 
 package buildbarn.http.oidc;
 
+import "google/protobuf/duration.proto";
 import "google/protobuf/timestamp.proto";
 import "pkg/proto/auth/auth.proto";
 
@@ -34,6 +35,10 @@ message CookieValue {
     // If set, a refresh token that may be used to obtain a new access
     // token and updated claims after the current access token expires.
     string refresh_token = 3;
+
+    // The expiration duration to use if the next token refresh does not
+    // return "expires_in" explicitly.
+    google.protobuf.Duration default_expiration = 4;
   }
 
   oneof session_state {