Weighted Cluster Retry Skips Target Cluster RBAC

Affected commit: c33d01624d
Sink: source/common/router/router.cc:2395

Summary

When cross-cluster retry is enabled, Envoy can authorize a request using one weighted cluster's permissive RBAC policy, then retry it against a different weighted cluster without reapplying that cluster's stricter policy. The code comment at line 2391-2394 explicitly acknowledges this: "part of initial cluster's configuration like circuit breaking, retry policy and so on will still be used."

Detail

// source/common/router/router.cc:2388-2396
if (cross_cluster_retry_) {
  // TODO(wbpcode): In current implementation, although we will refresh the
  // target upstream cluster for this retry attempt. But part of initial
  // cluster's configuration like circuit breaking, retry policy and so on
  // will still be used for this request because it will bring lots of
  // complexity to refresh all these state and bring limited benefit.
  callbacks_->downstreamCallbacks()->refreshRouteCluster();
}

If the initial cluster has a permissive per-route RBAC policy and the retry target cluster has a strict policy, the request is forwarded using the initial cluster's authorization decision.

Reproduce

#!/bin/bash
set -e

git clone --depth 1 https://github.com/envoyproxy/envoy.git /tmp/envoy-retry-rbac
cd /tmp/envoy-retry-rbac
echo "HEAD: $(git rev-parse HEAD)"

# Show the cross-cluster retry code with the explicit TODO acknowledging
# that initial cluster config (including authorization) is reused
sed -n '2385,2400p' source/common/router/router.cc

echo ""
echo "=== Vulnerable code confirmed at HEAD ==="
echo "router.cc:2388-2396: cross_cluster_retry_ refreshes the route cluster"
echo "but the TODO at line 2391-2394 explicitly states that 'part of initial"
echo "cluster's configuration like circuit breaking, retry policy and so on"
echo "will still be used.' This includes per-route RBAC decisions."
echo ""
echo "Attack scenario:"
echo "  1. Weighted route has clusters A (permissive RBAC) and B (strict RBAC)"
echo "  2. Request is authorized against cluster A's policy"
echo "  3. Cluster A returns a retriable error"
echo "  4. Retry selects cluster B via refreshRouteCluster()"
echo "  5. Cluster B's stricter RBAC is NOT reapplied"
echo "  6. Request is forwarded to cluster B under cluster A's authorization"

rm -rf /tmp/envoy-retry-rbac

Observed output (verified at c33d01624d):

HEAD: c33d01624d5b173b5d6e5c0c0474d480cab69b7b
    host_selection_cancelable_.reset();
  }

  if (cross_cluster_retry_) {
    // If the cross cluster retry is enabled, we need to refresh the route cluster for this attempt.
    //
    // TODO(wbpcode): In current implementation, although we will refresh the target upstream
    // cluster for this retry attempt. But part of initial cluster's configuration like circuit
    // breaking, retry policy and so on will still be used for this request because it will bring
    // lots of complexity to refresh all these state and bring limited benefit.
    callbacks_->downstreamCallbacks()->refreshRouteCluster();
  }

  // Clusters can technically get removed by CDS during a retry. Make sure it still exists.
  const auto cluster = config_->cm_.getThreadLocalCluster(route_entry_->clusterName());

=== Vulnerable code confirmed at HEAD ===
router.cc:2388-2396: cross_cluster_retry_ refreshes the route cluster
but the TODO at line 2391-2394 explicitly states that 'part of initial
cluster's configuration like circuit breaking, retry policy and so on
will still be used.' This includes per-route RBAC decisions.

Attack scenario:
  1. Weighted route has clusters A (permissive RBAC) and B (strict RBAC)
  2. Request is authorized against cluster A's policy
  3. Cluster A returns a retriable error
  4. Retry selects cluster B via refreshRouteCluster()
  5. Cluster B's stricter RBAC is NOT reapplied
  6. Request is forwarded to cluster B under cluster A's authorization

Credit

Zheng Yu @ Depthfirst