Fix: oauth2 state encoding (envoyproxy#37473)

Commit Message:
* Changes the state pramater to a json object to store the user state
before redirecting the user request to the auth server. Currently, the
json object has two fields: the original url and a nonce for csrf
prevention.
* Changes the state econding to
[Base64URL](https://datatracker.ietf.org/doc/html/rfc4648#section-5) to
fix [ AWS cognito doesn't support url encoded state
value](https://docs.aws.amazon.com/cognito/latest/developerguide/authorization-endpoint.html#get-authorize-request-parameters)
* Change the nonce from a plain timestamp to a random number to enhance
its security robustness. This addresses the "1. Generation" in this
issue envoyproxy#36276
* Small refacor: rename variables related to token_secret to hmac_secret
to improve clarity and consistency.

Example:
original state:
`{"url":"https://localhost:8080/login","nonce":"IPOom6PfIoFS+MmiV04aTJai8vUYlzyO5zUgT2G8mZA="}`
base64url encoded state:
`eyJ1cmwiOiJodHRwczovL2xvY2FsaG9zdDo4MDgwL2xvZ2luIiwibm9uY2UiOiJJUE9vbTZQZklvRlMrTW1pVjA0YVRKYWk4dlVZbHp5TzV6VWdUMkc4bVpBPSJ9`

Additional Description: The nonce in the [state parameter
](https://datatracker.ietf.org/doc/html/rfc6749#section-10.12)is used
for csrf prevention and is applicable for both oauth2 and oidc. Please
note that the OIDC spec defines a seperate [nonce
parameter](https://openid.net/specs/openid-connect-core-1_0.html#AuthRequest)
,which is specifically designed to prevent replay attacks and is unique
to OIDC.

More discussion about state and nonce parameters and be found in this
comment:
envoyproxy#37050 (comment)

Risk Level:
Testing: Unit test and integration test
Docs Changes:
Release Notes: Yes
Platform Specific Features:
[Optional Runtime guard:] A runtime gurad
"envoy.reloadable_features.oauth2_enable_state_nonce" has been added for
the new nonce in the state parameter.
[Optional Fixes envoyproxy#37049 envoyproxy#36871]
[Optional Fixes commit #PR or SHA]
[Optional Deprecated:]
[Optional [API
Considerations](https://github.com/envoyproxy/envoy/blob/main/api/review_checklist.md):]

Related Envoy Gateway issue:
envoyproxy/gateway#4625

I've tested this PR against AWS cognito using Envoy Gateway
SecurityPolicy, and it worked.

cc @missBerg @arkodg

---------

Signed-off-by: Huabing Zhao <[email protected]>
Signed-off-by: code <[email protected]>
Co-authored-by: code <[email protected]>
Co-authored-by: phlax <[email protected]>
(cherry picked from commit 0c6a1d2)
Signed-off-by: Huabing Zhao <[email protected]>

changelogs/current.yaml

@@ -6,6 +6,81 @@ behavior_changes:
 minor_behavior_changes:
 # *Changes that may cause incompatibilities for some users, but should not for most*
 
+- area: access_log
+  change: |
+    New implementation of the JSON formatter will be enabled by default.
+    The :ref:`sort_properties <envoy_v3_api_field_config.core.v3.JsonFormatOptions.sort_properties>` field will
+    be ignored in the new implementation because the new implementation always sorts properties. And the new implementation
+    will always keep the value type in the JSON output. For example, the duration field will always be rendered as a number
+    instead of a string.
+    This behavior change could be disabled temporarily by setting the runtime
+    ``envoy.reloadable_features.logging_with_fast_json_formatter`` to false.
+- area: xds
+  change: |
+    A minor delta-xDS optimization that avoids copying resources when ingesting them was introduced.
+    No impact to the behavior is expected, but a runtime flag was added as this may impact config-ingestion
+    related extensions (e.g., custom-config-validators, config-tracker), as the order of the elements passed
+    to the callback functions may be different. This change can be temporarily reverted
+    by setting ``envoy.reloadable_features.xds_prevent_resource_copy`` to ``false``.
+- area: formatter
+  change: |
+    The NaN and Infinity values of float will be serialized to ``null`` and ``"inf"`` respectively in the
+    metadata (``DYNAMIC_METADATA``, ``CLUSTER_METADATA``, etc.) formatter.
+- area: sds
+  change: |
+    Relaxed the backing cluster validation for Secret Discovery Service(SDS). Currently, the cluster that supports SDS,
+    needs to be a primary cluster i.e. a non-EDS cluster defined in bootstrap configuration. This change relaxes that
+    restriction i.e. SDS cluster can be a dynamic cluster. This change is enabled by default, and can be reverted by setting
+    the runtime flag ``envoy.restart_features.skip_backing_cluster_check_for_sds`` to ``false``.
+- area: http
+  change: |
+    If the :ref:`pack_trace_reason <envoy_v3_api_field_extensions.request_id.uuid.v3.UuidRequestIdConfig.pack_trace_reason>`
+    is set to false, Envoy will not parse the trace reason from the ``x-request-id`` header to ensure reads and writes of
+    trace reason be consistant.
+    If the :ref:`pack_trace_reason <envoy_v3_api_field_extensions.request_id.uuid.v3.UuidRequestIdConfig.pack_trace_reason>`
+    is set to true and external ``x-request-id`` value is used, the trace reason in the external request id will not
+    be trusted and will be cleared.
+- area: oauth2
+  change: |
+    :ref:`use_refresh_token <envoy_v3_api_field_extensions.filters.http.oauth2.v3.OAuth2Config.use_refresh_token>`
+    is now enabled by default. This behavioral change can be temporarily reverted by setting runtime guard
+    ``envoy.reloadable_features.oauth2_use_refresh_token`` to false.
+- area: oauth2
+  change: |
+    The ``state`` parameter in the OAuth2 authorization request has been changed to a base64url-encoded JSON object.
+    The JSON object contains the original request URL and a nonce for CSRF prevention.
+- area: quic
+  change: |
+    Enable UDP GRO in QUIC client connections by default. This behavior can be reverted by setting
+    the runtime guard ``envoy.reloadable_features.prefer_quic_client_udp_gro`` to false.
+- area: scoped_rds
+  change: |
+    The :ref:`route_configuration <envoy_v3_api_field_config.route.v3.ScopedRouteConfiguration.route_configuration>` field
+    is supported when the ``ScopedRouteConfiguration`` resource is delivered via SRDS.
+- area: http
+  change: |
+    Local replies now traverse the filter chain if 1xx headers have been sent to the client. This change can be reverted
+    by setting the runtime guard ``envoy.reloadable_features.local_reply_traverses_filter_chain_after_1xx`` to false.
+- area: dns
+  change: |
+    Patched c-ares to address CVE-2024-25629.
+- area: csrf
+  change: |
+    Increase only the statistics counter ``missing_source_origin`` for requests with a missing source origin.
+    Previously, the ``request_invalid`` counter was also increased for such requests.
+- area: rate_limit
+  change: |
+    add ``WEEK`` to the unit of time for rate limit.
+- area: rds
+  change: |
+    When a new RDS provider config is pushed via xDS and the only difference is change to
+    :ref:`initial_fetch_timeout <envoy_v3_api_field_config.core.v3.ConfigSource.initial_fetch_timeout>`,
+    the already existing provider will be reused. Envoy will not ask RDS server for routes
+    config because existing provider already has up to date routes config.
+    This behavioral change can be temporarily reverted by setting runtime guard
+    ``envoy.reloadable_features.normalize_rds_provider_config`` to false.
+>>>>>>> 0c6a1d2b84 (Fix: oauth2 state encoding (#37473))
+
 bug_fixes:
 # *Changes expected to improve the state of the world and are unlikely to have negative effects*
 

source/extensions/filters/http/oauth2/BUILD

@@ -48,6 +48,7 @@ envoy_cc_library(
         ":oauth_client",
         "//envoy/server:filter_config_interface",
         "//source/common/common:assert_lib",
+        "//source/common/common:base64_lib",
         "//source/common/common:empty_string",
         "//source/common/common:matchers_lib",
         "//source/common/crypto:utility_lib",

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

@@ -47,15 +47,15 @@ Http::FilterFactoryCb OAuth2Config::createFilterFactoryFromProtoTyped(
   const auto& proto_config = proto.config();
   const auto& credentials = proto_config.credentials();
 
-  const auto& token_secret = credentials.token_secret();
+  const auto& client_secret = credentials.token_secret();
   const auto& hmac_secret = credentials.hmac_secret();
 
   auto& cluster_manager = context.serverFactoryContext().clusterManager();
   auto& secret_manager = cluster_manager.clusterManagerFactory().secretManager();
   auto& transport_socket_factory = context.getTransportSocketFactoryContext();
-  auto secret_provider_token_secret = secretsProvider(
-      token_secret, secret_manager, transport_socket_factory, context.initManager());
-  if (secret_provider_token_secret == nullptr) {
+  auto secret_provider_client_secret = secretsProvider(
+      client_secret, secret_manager, transport_socket_factory, context.initManager());
+  if (secret_provider_client_secret == nullptr) {
     throw EnvoyException("invalid token secret configuration");
   }
   auto secret_provider_hmac_secret =
@@ -72,7 +72,7 @@ Http::FilterFactoryCb OAuth2Config::createFilterFactoryFromProtoTyped(
   }
 
   auto secret_reader = std::make_shared<SDSSecretReader>(
-      std::move(secret_provider_token_secret), std::move(secret_provider_hmac_secret),
+      std::move(secret_provider_client_secret), std::move(secret_provider_hmac_secret),
       context.serverFactoryContext().threadLocal(), context.serverFactoryContext().api());
   auto config = std::make_shared<FilterConfig>(proto_config, context.serverFactoryContext(),
                                                secret_reader, context.scope(), stats_prefix);
@@ -83,7 +83,8 @@ Http::FilterFactoryCb OAuth2Config::createFilterFactoryFromProtoTyped(
             std::make_unique<OAuth2ClientImpl>(cluster_manager, config->oauthTokenEndpoint(),
                                                config->retryPolicy(), config->defaultExpiresIn());
         callbacks.addStreamFilter(std::make_shared<OAuth2Filter>(
-            config, std::move(oauth_client), context.serverFactoryContext().timeSource()));
+            config, std::move(oauth_client), context.serverFactoryContext().timeSource(),
+            context.serverFactoryContext().api().randomGenerator()));
       };
 }