oauth2: fix Max-Age attribute of Set-Cookie response header

changelogs/current.yaml

@@ -1,14 +1,24 @@
-date: April 5, 2023
+date: April 13, 2023
 
 behavior_changes:
 - area: http
   change: |
     Validate upstream request header names and values. The new runtime flag ``envoy.reloadable_features.validate_upstream_headers`` can be used for revert this behavior.
+- area: oauth2
+  change: |
+    The Max-Age attribute of Set-Cookie HTTP response header was fixed, and now it is being assigned a value
+    representing the number of seconds until the cookie expires. This behavioral change can be reverted by
+    setting runtime guard ``envoy.reloadable_features.oauth_use_standard_max_age_value`` to false.
 
 bug_fixes:
 - area: oauth2
   change: |
     fixed a bug where the oauth2 filter would crash if it received a redirect URL without a state query param set.
+- area: oauth2
+  change: |
+    The Max-Age attribute of Set-Cookie HTTP response header was being assigned a value representing Seconds Since
+    the Epoch, causing cookies to expire in ~53 years. This was fixed an now it is being assinged a value representing
+    the number of seconds until the cookie expires.
 - area: lua
   change: |
     lua coroutine should not execute after local reply is sent.

source/common/runtime/runtime_features.cc

@@ -60,6 +60,7 @@ RUNTIME_GUARD(envoy_reloadable_features_lua_respond_with_send_local_reply);
 RUNTIME_GUARD(envoy_reloadable_features_no_extension_lookup_by_name);
 RUNTIME_GUARD(envoy_reloadable_features_no_full_scan_certs_on_sni_mismatch);
 RUNTIME_GUARD(envoy_reloadable_features_oauth_header_passthrough_fix);
+RUNTIME_GUARD(envoy_reloadable_features_oauth_use_standard_max_age_value);
 RUNTIME_GUARD(envoy_reloadable_features_original_dst_rely_on_idle_timeout);
 RUNTIME_GUARD(envoy_reloadable_features_postpone_h3_client_connect_to_next_loop);
 RUNTIME_GUARD(envoy_reloadable_features_quic_defer_send_in_response_to_packet);

source/extensions/filters/http/oauth2/filter.cc

@@ -451,6 +451,7 @@ void OAuth2Filter::onGetAccessTokenSuccess(const std::string& access_code,
   access_token_ = access_code;
   id_token_ = id_token;
   refresh_token_ = refresh_token;
+  expires_in_ = std::to_string(expires_in.count());
 
   const auto new_epoch = time_source_.systemTime() + expires_in;
   new_expires_ = std::to_string(
@@ -476,10 +477,17 @@ void OAuth2Filter::finishFlow() {
   std::string encoded_token;
   absl::Base64Escape(pre_encoded_token, &encoded_token);
 
+  std::string* max_age;
+  if (Runtime::runtimeFeatureEnabled(
+          "envoy.reloadable_features.oauth_use_standard_max_age_value")) {
+    max_age = &expires_in_;
+  } else {
+    max_age = &new_expires_;
+  }
+
   // We use HTTP Only cookies for the HMAC and Expiry.
-  const std::string cookie_tail = fmt::format(CookieTailFormatString, new_expires_);
-  const std::string cookie_tail_http_only =
-      fmt::format(CookieTailHttpOnlyFormatString, new_expires_);
+  const std::string cookie_tail = fmt::format(CookieTailFormatString, *max_age);
+  const std::string cookie_tail_http_only = fmt::format(CookieTailHttpOnlyFormatString, *max_age);
 
   // At this point we have all of the pieces needed to authorize a user.
   // Now, we construct a redirect request to return the user to their

source/extensions/filters/http/oauth2/filter.h

@@ -248,6 +248,7 @@ class OAuth2Filter : public Http::PassThroughDecoderFilter, public FilterCallbac
   std::string access_token_; // TODO - see if we can avoid this being a member variable
   std::string id_token_;
   std::string refresh_token_;
+  std::string expires_in_;
   std::string new_expires_;
   absl::string_view host_;
   std::string state_;

test/extensions/filters/http/oauth2/filter_test.cc

@@ -946,6 +946,73 @@ TEST_F(OAuth2Test, OAuthTestFullFlowPostWithParameters) {
   filter_->finishFlow();
 }
 
+/**
+ * Testing oauth response after tokens are set.
+ *
+ * Expected behavior: cookies are set.
+ */
+TEST_F(OAuth2Test, OAuthAccessTokenSucessWithTokens) {
+
+  // Set SystemTime to a fixed point so we get consistent HMAC encodings between test runs.
+  test_time_.setSystemTime(SystemTime(std::chrono::seconds(1000)));
+
+  // Expected response after the callback is complete.
+  Http::TestRequestHeaderMapImpl expected_headers{
+      {Http::Headers::get().Status.get(), "302"},
+      {Http::Headers::get().SetCookie.get(),
+       "OauthHMAC="
+       "MjI2YmI5YTRiZjJlNTFlNDUzZWVjOWUzYmU1MThlNGQyNDgyNzA0ZTBkMGQyY2M3M2QyMzg3NTRkZTY0YmU5YQ==;"
+       "version=1;path=/;Max-Age=600;secure;HttpOnly"},
+      {Http::Headers::get().SetCookie.get(),
+       "OauthExpires=1600;version=1;path=/;Max-Age=600;secure;HttpOnly"},
+      {Http::Headers::get().SetCookie.get(),
+       "BearerToken=access_code;version=1;path=/;Max-Age=600;secure"},
+      {Http::Headers::get().SetCookie.get(),
+       "IdToken=some-id-token;version=1;path=/;Max-Age=600;secure"},
+      {Http::Headers::get().SetCookie.get(),
+       "RefreshToken=some-refresh-token;version=1;path=/;Max-Age=600;secure"},
+      {Http::Headers::get().Location.get(), ""},
+  };
+
+  EXPECT_CALL(decoder_callbacks_, encodeHeaders_(HeaderMapEqualRef(&expected_headers), true));
+
+  filter_->onGetAccessTokenSuccess("access_code", "some-id-token", "some-refresh-token",
+                                   std::chrono::seconds(600));
+}
+
+TEST_F(OAuth2Test, OAuthAccessTokenSucessWithTokens_oauth_use_standard_max_age_value) {
+  TestScopedRuntime scoped_runtime;
+  scoped_runtime.mergeValues({
+      {"envoy.reloadable_features.oauth_use_standard_max_age_value", "false"},
+  });
+
+  // Set SystemTime to a fixed point so we get consistent HMAC encodings between test runs.
+  test_time_.setSystemTime(SystemTime(std::chrono::seconds(0)));
+
+  // Expected response after the callback is complete.
+  Http::TestRequestHeaderMapImpl expected_headers{
+      {Http::Headers::get().Status.get(), "302"},
+      {Http::Headers::get().SetCookie.get(),
+       "OauthHMAC="
+       "OTBhMzEwNjk4YzJiNjIxMTcwMTE0ZDE2NjUyNjIyNmI1YmE0Y2NhNTQ3ZWYwZGYzZTNhYjEwNzhmZmQyZGY4Zg==;"
+       "version=1;path=/;Max-Age=600;secure;HttpOnly"},
+      {Http::Headers::get().SetCookie.get(),
+       "OauthExpires=600;version=1;path=/;Max-Age=600;secure;HttpOnly"},
+      {Http::Headers::get().SetCookie.get(),
+       "BearerToken=access_code;version=1;path=/;Max-Age=600;secure"},
+      {Http::Headers::get().SetCookie.get(),
+       "IdToken=some-id-token;version=1;path=/;Max-Age=600;secure"},
+      {Http::Headers::get().SetCookie.get(),
+       "RefreshToken=some-refresh-token;version=1;path=/;Max-Age=600;secure"},
+      {Http::Headers::get().Location.get(), ""},
+  };
+
+  EXPECT_CALL(decoder_callbacks_, encodeHeaders_(HeaderMapEqualRef(&expected_headers), true));
+
+  filter_->onGetAccessTokenSuccess("access_code", "some-id-token", "some-refresh-token",
+                                   std::chrono::seconds(600));
+}
+
 TEST_F(OAuth2Test, OAuthBearerTokenFlowFromHeader) {
   Http::TestRequestHeaderMapImpl request_headers{
       {Http::Headers::get().Path.get(), "/test?role=bearer"},