Istio DECODE_AND_MERGE_SLASHES option breaks GitLab

[corang]

Steps to reproduce

1. Make a request to gitlab like api/v4/projects/namespace%2Fproject through the istio tenant gateway

Expected result

Project info is received

Actual Result

Gitlab serves a 404 since it's expecting the url to not have been decoded

Visual Proof (screenshots, videos, text, etc)

Severity/Priority

Medium: at least gitlab web ide is broken, but any other api requests that need encoded slashes are too

Additional Context

Add any other context or screenshots about the technical debt here.

[mjnagel]

This appears to be a result of the path normalization we added to the istio mesh config. At a glance I'm not sure this can be configured per workload so it may be difficult to enable it for keycloak, but not gitlab.

Context on why we have this is essentially this doc, related to our usage of AuthorizationPolicy resources to protect certain keycloak endpoints. We may need to evaluate other options here if we can't do per-workload normalization, or identify if there is a way to make Gitlab happy with normalized paths.

Edit: More context - Gitlab has an open issue regarding this, people have encountered it with other proxies. The TLDR is they don't plan to change anything, but may add docs to note this being a requirement for your proxy to not decode URLs.

[mjnagel]

After doing some reading I was hoping that an envoyfilter like this might work:

apiVersion: networking.istio.io/v1alpha3
kind: EnvoyFilter
metadata:
  name: disable-path-normalization
  namespace: gitlab
spec:
  configPatches:
  - applyTo: NETWORK_FILTER
    match:
      context: ANY
      listener:
        filterChain:
          filter:
            name: envoy.filters.network.http_connection_manager
    patch:
      operation: MERGE
      value:
        typed_config:
          '@type': 'type.googleapis.com/envoy.extensions.filters.network.http_connection_manager.v3.HttpConnectionManager'
          "normalize_path": false
          "merge_slashes": false

However it appears that merges don't overwrite the bools that would be set to true in this case (see istio/istio#18169) so this doesn't work as I would hope. There are other operations we could use but that would likely mean recreating the full http connection manager config.

What might be possible is flipping the mesh config back to non-normalized paths and using the envoyfilter to normalize keycloak specifically. I'm not sure on the implications of that and would defer to @bburky and @jeff-mccoy if there's any downside to normalizing via envoyfilter, only for keycloak (assuming the above envoyfilter or similar would actually work)? I'm also not 100% sure - we might be impacted by the same issue in istio merging those boolean fields.

[bburky]

Currently we're only trying to enforce policies on Keycloak and the AuthorizationPolicy is enforced at the workload not the gateway. So yes, we could theoretically only enforce policies at the Keycloak istio sidecar. If this EnvoyFilter works for enabling on Keycloak only, I guess this is fine.

If we start to use more path-based AuthorizationPolicies we'll need one of these EnvoyFilters in each namespace, but so far it's just Keycloak.

We could also downgrade the normalization level to MERGE_SLASHES, but I'd rather not.

[Racer159]

FWIW we may run into this issue more often with GitLab since many services (NPM coming to mind) expect a %2F to namespace something (in NPMs case it is something like @types%2fnode)

[bburky]

To address CVEs promptly in UDS packages, I'd like to have the option to mitigate CVEs in packages with DENY AuthorizationPolicies. If we can securely mitigate a CVE by disabling an affected API, sometimes we can wait update and patch. Also sometimes it's hard to do a full update of a package, so shipping a mitigation ASAP while we take time to update is a nice option.

For example, this recent Grafana CVE uses a (rarely used I think?) /api/datasources API. I think a DENY AuthorizationPolicy on /api/datasources would have mitigated this.

The reason I mention this is because, if possible, I'd like to have DECODE_AND_MERGE_SLASHES enabled on as many namespaces as possible, otherwise path based AuthorizationPolicies can likely be bypassed. If it's just GitLab we skip, that's fine. But I'd really like it enabled on all namespaces if possible. (If we have to use an EnvoyFilter, we could maybe use Pepr to generate it in each namespace?)

[bburky]

Let's try the opposite of the previous EnvoyFilter: have UDS operator generate an EnvoyFilter per-namespace, opting into DECODE_AND_MERGE_SLASHES behavior. The UDS CR will have a setting defaulting to path normalization, but specific apps (GitLab) can opt out.

Something along the lines of:

apiVersion: networking.istio.io/v1alpha3
kind: EnvoyFilter
metadata:
  name: path-normalization
  namespace: ${namespace}
spec:
  configPatches:
  - applyTo: NETWORK_FILTER
    match:
      context: ANY
      listener:
        filterChain:
          filter:
            name: envoy.filters.network.http_connection_manager
    patch:
      operation: MERGE
      value:
        typed_config:
          '@type': 'type.googleapis.com/envoy.extensions.filters.network.http_connection_manager.v3.HttpConnectionManager'
          "normalize_path": true
          "merge_slashes": true
          "path_with_escaped_slashes_action": 4

I think the list of envoy settings configured by Istio with DECODE_AND_MERGE_SLASHES are:
https://github.com/istio/istio/blob/e8720551beb31790ae89077bc31c9372aeec5356/pilot/pkg/networking/core/listener_builder.go#L326-L329
The safest way to get the right config is probably enabling DECODE_AND_MERGE_SLASHES, and then dumping the proxy-config from a sidecar.
Please verify correct normalization using all the URLs from Istio's integration tests. Try using curl --path-as-is or similar against podinfo and review the log. Or however you want to test.
https://github.com/istio/istio/blob/e8720551beb31790ae89077bc31c9372aeec5356/tests/integration/security/normalization_test.go#L160-L181

A couple questions:

• There are some other per-namespace resources it may make sense to generate with UDS operator (we've discussd AuthorizationPolicies for authservice). I'm not sure if it makes sense to design some generic resource generation code in UDS operator now? As long as we don't change the external API, we can modify the internal code later
• The external API can probably be a nil-able boolean on the UDS CR? pathNormalization: false or something? Unless we want an enum to allow configuration in the future? Default (nil) behavior should be the equivalent of DECODE_AND_MERGE_SLASHES
• One issue with this approach: Istio gets enabled for the whole cluster except whatever small number of namespaces opt out. An istio meshconfig setting would apply to every namespace with istio. There are probably some namespaces with istio, but no UDS CR. This approach will not apply normalization to those
  • This is honestly probably a broader issue than Istio specifically. Like, we'd want to generate NetworkPolicies in all namespaces someday probably? Someday we should audit for namespaces without UDS CRs maybe?
• We need to not do path normalization at the gateway, or apps downstream of it can't really opt out if the path has already been normalized. This is fine, don't do normalization at the gateway. We do need to make sure all AuthorizationPolices we write in the future with path-based policies are only enforced at the sidecar (where normalization is done) though.

@mjnagel If you want to do a PR to implement this that would be great.

[mjnagel]

Commenting here for posterity - after further review of keycloak and the envoyfilter solution we are pivoting to just using MERGE_SLASHES. Keycloak is not affected by encoded urls (does not automatically decode them) so this doesn't reduce the security of our authpolicies.

This does however mean that we may not be able to confidently and quickly protect against CVEs affecting specific endpoints - we would need to review and validate each specific app to ensure they don't decode URLs, and potentially add an envoyfilter to do further modification, none of which could be done quickly in response to a CVE.