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

app access sends incorrect http2 frame for GET requests #23740

Open
programmerq opened this issue Mar 28, 2023 · 2 comments
Open

app access sends incorrect http2 frame for GET requests #23740

programmerq opened this issue Mar 28, 2023 · 2 comments
Labels
application-access bug c-cc Internal Customer Reference c-nx Internal Customer Reference regression

Comments

@programmerq
Copy link
Contributor

programmerq commented Mar 28, 2023

Expected behavior:

When using http2, the app access service should set the end_stream flag on the frame with the headers.

Current behavior:

When using http2, app access does not set the end_stream on a simple GET request that has only headers. This makes some upstream components error out in unexpected ways. Without the end_stream flag being set, the http2 server expects more data to come in a subsequent frame.

In particular, this causes difficulties when app access sends the request to another reverse proxy such as an ALB or an envoy endpoint. Both of those will treat the request like there is more data to come, and set the Transfer-Encoding: chunked header when it sends the request further upstream. Since the Transfer-Encoding: chunked header is not allowed on a simple GET request since it has no body, this request may well be rejected altogether.

Bug Details

Teleport version

10.2.2 and up

This is a regression. I have tested the following versions:

  • 10.2.0 (works correctly)
  • 10.2.1 (works correctly)
  • 10.2.2 (triggers the problem)
  • 10.2.4 (triggers the problem)
  • 11.x.y (all affected)
  • 12.x.y (all work correctly)

The version of the agent seems to be the deciding factor.

Recreation steps

Set up an http2 capable endpoint. envoy is good since it has the ability to really turn up debugging

Configure a teleport app access app to connect to the envoy https endpoint

Send a request over app access

Since envoy has good debugging, its logs will indicate that the end_stream is missing, and any upstream app from there will have the Transfer-Encoding: chunked header present.

I've included an example envoy.yaml (generate a self-signed cert), a teleport.yaml (app access agent), and the logs from envoy when sending the request through my lap setup.

envoy.yaml - ran as envoy -c /etc/envoy/envoy.yaml --log-level trace
envoylogs.txt
teleport.yaml

In particular, the [2023-03-28 21:55:23.742][19][debug][http] [source/common/http/conn_manager_impl.cc:972] [C38][S16903837606675993007] request headers complete (end_stream=false): log line indicates a problem. When using another http2 client to send traffic to envoy, this log indicates end_stream=true

@programmerq programmerq added bug regression application-access c-nx Internal Customer Reference c-cc Internal Customer Reference labels Mar 28, 2023
@programmerq
Copy link
Contributor Author

Update: I ran an app access agent in a debugger and tracked down the cause.

#16372 was added to the v10 branch. The tracing wrapper that was introduced confuses the go http2 data frame encoder. The open-telemetry/opentelemetry-go-contrib#2983 upstream PR fixes the problem. That fix was first available in go.opentelemetry.io/contrib/instrumentation/net/http/otelhttp 0.37 and up

All of the affected versions of Teleport have an older version of that dependency. As of Teleport 12.0.0 and newer, Teleport has a fixed version of the dependency.

I think it would make sense to bump the dependency version for branch/v10 and branch/v11 to resolve this.

rosstimothy added a commit that referenced this issue Mar 29, 2023
Bumps go.opentelemetry.io/contrib/instrumentation/google.golang.org/grpc/otelgrpc
and go.opentelemetry.io/contrib/instrumentation/net/http/otelhttp to
v0.40.0.

Fixes #23740
@rosstimothy
Copy link
Contributor

Updating otelhttp on branch/v10 and branch/11 looks like it might not be viable. If we update just otelhttp and none of the other otel libraries then the build breaks due to open-telemetry/opentelemetry-go#3548.

OTel Go packages are compatible only between the versions that were released from the same commit

However, updating all other otel dependencies pulls in quite a few other dependencies as seen here. Most notably it pulls in cloud.google.com/go/firestore v1.9.0, which introduces a deprecation warning of the Firestore APIs that we use on all current release branches. In order to update this we either need to backport #21190 or disable linting on said deprecation warnings.

Some alternate solutions could be to:

  1. Downgrade otel to a version that respects NoBody
  2. Disable http auto instrumentation for app access on v10 and v11

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
application-access bug c-cc Internal Customer Reference c-nx Internal Customer Reference regression
Projects
None yet
Development

No branches or pull requests

2 participants