Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Request with | (pipe) character in authority header is auto-rejected by Limitador #53

Closed
rahulanand16nov opened this issue Jan 11, 2022 · 12 comments

Comments

@rahulanand16nov
Copy link
Contributor

This issue describes the finding of auto-rejection of requests that contains the pipe character in their :authority header field. I tracked down this problem to a dependency named h2.

Following was from the trace logs:

malformed headers: malformed authority (b"outbound|8081||limitador.rahul-test.svc.cluster.local"): invalid uri character

Why there is a pipe character in the authority header?
That format should be familiar if you have seen Istio's cluster config. Still, if not, that format is auto-generated by Istio's control plane by looking at its service registry. So, when you add a Limitador service in the cluster where Istio is also present, the service registry picks up the service and sends that to all the Envoys present in the cluster (gateway or sidecar) as a cluster. And the name of that cluster is of that format as seen above in the log.

As Limitador is used right now, it receives a callout request from an Envoy's RateLimit filter and one particular behavior of envoy is to use the cluster name as :authority header of the callout. Hence why you get the pipe character in the header.

Potential way to solve this problem?
I have seen a few comments around updating dependencies but I cannot find them right now. One option is to update the dependency which is already a bit old.

Second, I am going to create an issue in the Istio and Envoy repo to highlight this behavior to them. Possibly Istio can change the cluster format or Envoy can allow overriding authority?

cc @maleck13 @unleashed

@unleashed
Copy link
Contributor

@rahulanand16nov I don't think it is feasible for Istio to change the cluster name format at this stage - it's been well documented for a long time. It might be easier to change Envoy to use the actual authority rather than the cluster name, but still difficult given how this has been around for long - so failing the above, we'll have to work around this case on our side.

@rahulanand16nov
Copy link
Contributor Author

Good point 🤔

I'll add steps to reproduce for anyone picking this up before I get to it.

@rahulanand16nov
Copy link
Contributor Author

cc @alexsnaps

@rahulanand16nov
Copy link
Contributor Author

rahulanand16nov commented May 30, 2022

I have tried using a DestinationRule which adds the TLS context to the cluster entry with SNI. Envoy swaps the authority header with SNI if present but limitador now rejects the request with a different error: connection error: http2 error: protocol error: unspecific protocol error detected

DestinationRule used:

apiVersion: networking.istio.io/v1beta1
kind: DestinationRule
metadata:
  name: limitador
  namespace: kuadrant-system
spec:
  host: limitador.kuadrant-system.svc.cluster.local
  trafficPolicy:
    portLevelSettings:
    - port:
        number: 8081
      tls:
        mode: SIMPLE
        sni: rate-limit-cluster

There are only two options left:

  1. Make a similar change to our dependency: Do not require authority for UNIX domain sockets hyperium/h2#487
  2. Just keep on using the current fix which is to add a custom cluster entry into Envoy with a name that will pass h2's restriction like rate-limit-cluster.

@alexsnaps
Copy link
Member

@alexsnaps
Copy link
Member

alexsnaps commented Jun 1, 2022

Found this issue that seems to suffer from the same malformed header:

2022-05-27T16:44:03.081190Z     debug   envoy http2     [C449] invalid http2: Invalid HTTP header field was received: frame type: 1, stream: 1, name: [:authority], value: [outbound|4317||otelcol-deployment-collector.opentelemetry.svc.cluster.local]
2022-05-27T16:44:03.081199Z     debug   envoy http2     [C449] invalid frame: Invalid HTTP header field was received on stream 1

@alexsnaps alexsnaps changed the title Request with pipe "|" character in authority header is auto-rejected by Limitador Request with | (pipe) character in authority header is auto-rejected by Limitador Jun 1, 2022
@rahulanand16nov
Copy link
Contributor Author

That's a very similar issue to ours. Added my thoughts there: istio/istio#39196 (comment)

@alexsnaps
Copy link
Member

I still can’t figure out where the fix should be tho… wondering if Envoy should somehow address this ¯_(ツ)_/¯

@rahulanand16nov
Copy link
Contributor Author

Changing Envoy will be a two-step change, first, we change the behavior in Envoy or we add another configurable field to the cluster config and then Istio follows suit to utilize that.

I think Istio should change the pipe to dot because virtually it has no change (or at least I cannot think of any problem).

@rahulanand16nov
Copy link
Contributor Author

Enabling TLS on the cluster forces Envoy to use SNI which follows the format of :authority header (authorino get's SNI version of the header). Maybe make sure Limitador accepts the TLS since right now it rejects the request with an unknown protocol or similar error can fix the issue.

@guicassolato
Copy link
Contributor

guicassolato commented Jun 8, 2022

@rahulanand16nov, Envoy gRPC client config allows to overwrite the :authority header (which by default comes from the cluster name, as you said): https://www.envoyproxy.io/docs/envoy/latest/api-v3/config/core/v3/grpc_service.proto#envoy-v3-api-msg-config-core-v3-grpcservice-envoygrpc.

Can you somehow influence Istio to use that option?

@maleck13
Copy link

@alexsnaps I am thinking we close this, do we expect to do anything about it? I will close and let it be reopened if needed

@maleck13 maleck13 closed this as not planned Won't fix, can't repro, duplicate, stale Feb 13, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Status: Done
Status: No status
Development

Successfully merging a pull request may close this issue.

5 participants