api: Add disableMergeSlash and escapedSlashesAction to ClientTrafficPolicy

[liorokman]

What this PR does / why we need it:

This PR adds settings into ClientTrafficPolicy to control slashes in the incoming request path URI.

The following settings are added:

spec:
  path:
     escapedSlashesAction: UnescapeRedirect # or UnescapeForward, RejectRequest, KeepUnchanged
     disableMergeSlashes: false # set to true to prevent Envoy from merging slashes 

The proposed defaults are the current EnvoyGateway behavior - so not setting these parameters will not cause any change to the current behavior.

These settings map to the EnvoyProxy HTTP Connection Manager configuration path_with_escaped_slashes_action and merge_slashes keys.

Which issue(s) this PR fixes:
These parameters were proposed in issue #1846 .

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (9be3a97) 64.65% compared to head (642cff7) 64.67%.
Report is 17 commits behind head on main.

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #2384      +/-   ##
==========================================
+ Coverage   64.65%   64.67%   +0.01%     
==========================================
  Files         114      115       +1     
  Lines       16618    16844     +226     
==========================================
+ Hits        10745    10894     +149     
- Misses       5194     5255      +61     
- Partials      679      695      +16     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[arkodg]

-1 on this setting, since its against the recommendations in https://www.envoyproxy.io/docs/envoy/latest/configuration/best_practices/edge#configuring-envoy-as-an-edge-proxy
what is the use case here @liorokman

[liorokman]

-1 on this setting, since its against the recommendations in https://www.envoyproxy.io/docs/envoy/latest/configuration/best_practices/edge#configuring-envoy-as-an-edge-proxy what is the use case here @liorokman

The mergeSlashes setting recommended by the best practices is actually not part of the HTTP spec. See the comment on the setting's documentation. There are instances of frameworks that require that proxies in the middle don't merge adjacent slashes - e.g. Passenger.

With regards to the escapedSlashesAction setting, the recommendations you cited actually recommend setting the value of this key based on your specific use-case:

Additionally the path_with_escaped_slashes_action setting should be set according to following recommendations:

REJECT_REQUEST if dowstream clients are expected to use RFC 3986 compliant normalized paths (i.e. gRPC clients).

UNESCAPE_AND_REDIRECT if downstream client supports HTTP redirect (i.e. a browser). This option minimizes possibility of path confusion by forcing request to be re-issued with the same path across all parties: downstream client, Envoy and upstream server. Note that gRPC requests will still be rejected with the INTERNAL (13) error code, as gRPC clients do not support redirect.

KEEP_UNCHANGED for servers that are not RFC 3986 compliant and require encoded slashes.

UNESCAPE_AND_FORWARD for servers that are known to treat escaped and unescaped slashes equivalently. Choosing this option may increase probablity of path confusion vulnerabilities if intermediaries perform path based access control.

In order to do so, it needs to be exposed in ClientTrafficPolicy.

In our use-case, we have some upstream servers that are not RFC 3986 compliant and we need to keep the encoded slashes as they are.

I think the current defaults are good, and I would definitely keep the current settings as defaults. But I think it does make sense to make these flags available for cases where the defaults don't work.

[liorokman]

A few other OSS ingress controllers also allow controlling the mergeSlashes key:

• Ambassador
• Contour (see "disableMergeSlashes" )
• Gloo (see "mergeSlashes")

[arkodg]

thanks for added context @liorokman, some thoughts

• since the above path field are set at within the hcm, we are limited to defining it in the ClientTrafficPolicy which might not be the ideal home, SecurityPolicy could have been another place to keep it, but can't how we can toggle this setting on/off at the route level
• prefer if we have a top level field like path to hold all path specific setting

path:
  disableEergeSlashes:

• prefer if escapedSlashesAction is represented as a boolinstead of enum e.g. allowEscapedSlashes

[liorokman]

• prefer if we have a top level field like path to hold all path specific setting

path:
  disableEergeSlashes:

These parameters are already grouped under a top level path field.

• prefer if escapedSlashesAction is represented as a boolinstead of enum e.g. allowEscapedSlashes

Changing this to a bool would mean that EG supports only two of the possible actions. It would only be possible to either KEEP_UNCHANGED or UNESCAPE_AND_REDIRECT. The documentation would have to explicitly say that if allowEscapedSlashes is set to false, the behavior is UNESCAPE_AND_REDIRECT and not any of the other options.

• Gloo allows the full range of actions supported by Envoy Proxy
• Ambassador allows choosing between KEEP_UNCHANGED and REJECT_REQUEST, and defaults to KEEP_UNCHANGED
• Contour leaves the default up to the underlying Envoy Proxy implementation, so it's really anyone's guess what actually happens at runtime.

I think there's also a hidden issue here that when the route is a gRPC route the better value for this configuration key would probably be REJECT_REQUEST, because gRPC clients are expected to be RFC 3986 compliant to begin with.

[arkodg]

prefer UnescapeAndRedirect

[liorokman]

Renamed

[arkodg]

prefer UnescapeAndForward

[liorokman]

Renamed

[arkodg]

thanks for the detailed comments @liorokman
only have suggestions reg naming, else LGTM

reg default setting for escapedSlashesAction for gRPC, based on the doc string for UNESCAPE_AND_REDIRECT. - Note that gRPC requests will still be rejected with the INTERNAL (13) error code, as gRPC clients do not support redirect.
looks like the outcome is the same as REJECT_REQUEST - error code 13 :)

[arkodg]

LGTM thanks for adding the extra details in the PR !