Ambassador: Crash when adding request header `x-forwarded-proto`

[schoren]

Describe the bug
I am trying to set the x-forwarded-proto header to the upstream server using add_request_headers config. When I access the URL, ambassador crashes.

Here's my mapping:

---
apiVersion: v1
kind: Service
metadata:
  name: myservice
  labels:
    app: myservice
  annotations:
    getambassador.io/config: |
      ---
      apiVersion: ambassador/v0
      kind: Mapping
      name: myservice
      prefix: /
      host: myservice.mydomain.com
      service: http://myservice.default:8088
      add_request_headers:
        x-forwarded-proto: "%PROTOCOL%"
spec:
  type: NodePort
  ports:
  - name: myservice
    port: 8088
    targetPort: 8088
  selector:
    app: myservice

To Reproduce
Steps to reproduce the behavior:

1. Configure a route as described above
2. Access the URL
3. See error

Expected behavior

I expected the service to get the x-forwarded-proto header, but instead ambassador crashes.

Versions (please complete the following information):

• Ambassador: 0.37.0
• Kubernetes environment: Azure Kubernetes Services (AKS)
• Version: v1.10.3

[frickenate]

Edit: Looks like an upstream bug with Envoy: https://github.com/envoyproxy/envoy/issues/3919.

---

Same issue here. Here's the crash log of the null pointer assertion that causes the crash. It's important to note that every time a request is received for the Mapping in question, Ambassador crashes. Thus a single Mapping configuration can make the entire k8s cluster unreachable via the gateway.

[dpankros]

In EA4, this crash no longer occurs, but the header is not modified either. An annotation like:

     apiVersion: ambassador/v0
      kind:  Mapping
      name:  httpbin_mapping
      prefix: /
      service: httpbin.org:80
      host_rewrite: httpbin.org
      add_request_headers:
        x-forwarded-proto: 'https'

Will still forwarded from envoy as:X-Forwarded-Proto: http (when connecting to ambassador/envoy over http)

[gsagula]

@containscafeine @schoren This a limitation in Envoy's version used in Ambassador: 0.37.0. However, we have recently released a patch to mitigate this problem. Can you upgrade it to Ambassador 0.40.1 and see if the problem persists please?

[gsagula]

@kflynn As per we chat via DM slack. Just to be clear, the patch released prevents existing request headers to be re-added.

@dpankros In recent Envoy versions, if XFP is in the original request or added by the ext-authz, whatever header value is added via add_request_headers will get appended. What maybe is happening in the case of EA4, is that the header gets appended but the http client only shows the first value (probably initially inserted by the filter since it is http). If you run Ambassador in debug mode you are able to see which headers the auth filter is inserting in the original request before dispatching it to the upstream.

Also, let's keep the discussion in this issue for tracking please.

[dpankros]

@gsagula I'm hitting the ambassador/envoy service directly with postman -- no XFP headers, just a bare http GET request. I do this this in the debug logs, however:

ACCESS [2018-11-07T21:21:52.319Z] "GET /headers?show_env=true HTTP/1.1" 200 - 0 775 87 85 "-" "PostmanRuntime/7.3.0" "8ad266c7-3640-491b-8ccd-fe99d025a331" "httpbin.org" "52.202.60.111:80"
[2018-11-07 21:22:40.774][103][error][config] source/common/http/header_map_impl.cc:373] addReferenceKey attempted to add an empty value
[2018-11-07 21:22:40.774][103][error][config] source/common/http/header_map_impl.cc:373] addReferenceKey attempted to add an empty value
[2018-11-07 21:22:40.774][103][error][config] source/common/http/header_map_impl.cc:330] insertByKey cannot append inline headers. Failed to insert key x-forwarded-proto
[2018-11-07 21:22:40.774][103][error][config] source/common/http/header_map_impl.cc:330] insertByKey cannot append inline headers. Failed to insert key x-forwarded-proto
[2018-11-07 21:22:40.774][103][error][config] source/common/http/header_map_impl.cc:373] addReferenceKey attempted to add an empty value

Seems that source/common/http/header_map_impl.cc:330] insertByKey cannot append inline headers. Failed to insert key x-forwarded-proto is probably a good place to start.

[gsagula]

Right, this is the patch that we released. Are you running it out of the
Ambassador 0.40.1?

Gabriel Linden Sagula

[dpankros]

Yes. My ambassador pod image shows: quay.io/datawire/ambassador:0.40.1

I should mention that 0.40 used to crash for me when adding XFP just like the @schoren, but that has been fixed for me in 0.40.1.

[gsagula]

Sure, that's all as expected. I'm curious about EA4. You said the the header is not added in this version as well. If that's right, try to run Ambassador with trace:

From Ambassador's pod:

$ curl -XPOST http://localhost:8001/logging?level=trace 

Then, try to find with headers are been added by the ext-authz filter. You will see a message like that:
https://github.com/envoyproxy/envoy/blob/bacaa4cdcbf94a8b110b783ae5e72cd2947a8432/source/extensions/filters/http/ext_authz/ext_authz.cc#L158

Headers added should be listed below. See if XFP is being added by the filter.

[dpankros]

I don't see anything from ext_authz outside of ambassador startup. E.g.:

[2018-11-10 22:17:19.889][14][info][main] source/server/server.cc:204] statically linked extensions:
[2018-11-10 22:17:19.889][14][info][main] source/server/server.cc:206]   access_loggers: envoy.file_access_log,envoy.http_grpc_access_log
[2018-11-10 22:17:19.889][14][info][main] source/server/server.cc:209]   filters.http: envoy.buffer,envoy.cors,envoy.ext_authz,envoy.fault,envoy.filters.http.header_to_metadata,envoy.filters.http.jwt_authn,envoy.filters.http.rbac,envoy.grpc_http1_bridge,envoy.grpc_json_transcoder,envoy.grpc_web,envoy.gzip,envoy.health_check,envoy.http_dynamo_filter,envoy.ip_tagging,envoy.lua,envoy.rate_limit,envoy.router,envoy.squash
[2018-11-10 22:17:19.889][14][info][main] source/server/server.cc:212]   filters.listener: envoy.listener.original_dst,envoy.listener.proxy_protocol,envoy.listener.tls_inspector
[2018-11-10 22:17:19.889][14][info][main] source/server/server.cc:215]   filters.network: envoy.client_ssl_auth,envoy.echo,envoy.ext_authz,envoy.filters.network.rbac,envoy.filters.network.sni_cluster,envoy.filters.network.thrift_proxy,envoy.http_connection_manager,envoy.mongo_proxy,envoy.ratelimit,envoy.redis_proxy,envoy.tcp_proxy
[2018-11-10 22:17:19.889][14][info][main] source/server/server.cc:217]   stat_sinks: envoy.dog_statsd,envoy.metrics_service,envoy.stat_sinks.hystrix,envoy.statsd
[2018-11-10 22:17:19.889][14][info][main] source/server/server.cc:219]   tracers: envoy.dynamic.ot,envoy.lightstep,envoy.zipkin
[2018-11-10 22:17:19.889][14][info][main] source/server/server.cc:222]   transport_sockets.downstream: envoy.transport_sockets.capture,raw_buffer,tls
[2018-11-10 22:17:19.889][14][info][main] source/server/server.cc:225]   transport_sockets.upstream: envoy.transport_sockets.capture,raw_buffer,tls

I've attached two traces so you can take a look. It's hard to see exactly where a new request starts and ends so I made a reasonable guesses.
Request through chrome:
ea4_trace.txt

Request through postman (probably cleaner):
ea4_trace_pm.txt

[gsagula]

@dpankros Thanks for the details. Looking at logs I can confirm that https has been appended 'x-forwarded-proto', 'http,https' and the HTTP client will probably just look at the first value https://tools.ietf.org/html/rfc7239#section-5.4

Would be useful to see the logs before that the router just to confirm if was the filter the one to first add the XFP header. Is XFP header in allowed_headers config?

[dpankros]

@gsagula I'm having a little difficulty understanding what you are asking for. I _think_ you're asking for router logs that occurred before the request logs that I had sent previously.

Here is a full log from ambassador.. If you need something else please let me know. I'm not aware of other logs, however, so if possible let me know where to get them and I'd be happy to oblige. I also did not see any instances of allowed_headers in the ambassador directory (or subdirs), other than:

ambassador-0.0.0.dev0-py3.6.egg-tmp/schemas/v0/AuthService.schema:        "allowed_headers": {
ambassador-demo-config/auth.yaml:  allowed_headers:

If you let me know where to look, I can supply any additional info you need. Thanks for your help!

[gsagula]

Thank you for the full logs. I can confirm that the http in the XFP header is not set by the filter and, in this case, it looks correct to me. May I understand the reason for adding a request header X-Forwarded-Proto: https considering that the client is not making a secure call?

[dpankros]

@gsagula Of course. The issue for us is that our devops architecture is roughly organized like:

Client <--> AWS ELB <---> Ambassador <---> Our App (k8s Service)

The ELB terminates the SSL and passes on XFP:https to ambassador. Ambassador then passes on XFP:http. If our app sees an indication that it didn't come in via https it issues a redirect to https. Resulting in a redirect loop.

In this case, the client request really is coming via https, it's just the intermediate connections to/from ambassador that are completely within our AWS VPC are not (for now). Unfortunately, disabling the redirect in our app also disables a bunch of useful features that we want to have enabled. Hence, if we could tell Ambassador to pass on XFP: https it avoids the redirect loop and solves the problem. I would like to use ambassador, but right now, we are forced to leave it out so that the ELB can pass the XFP: https directly to our app. That also means one ELB per environment, which gets expensive.

Understand our dillema?

[gsagula]

What you could try to do is to get rid of XFP in add_request_headers and add it to allowed_headers list. From the authz service, send the x-forwarded-proto: https which, in theory, I think will override the original x-forwarded-proto: http header. It's a big hack, but it might work.

[gsagula]

@dpankros Oh wait, I forgot that Ambassador Auth is not being used in your case, right?

[dpankros]

@gsagula No. I am not using it.

[gsagula]

Ambassador seems to be doing the right thing. Please, feel free to write a feature request if you find a potential solution for the use case described above. I will go ahead and close this issue for now. Thanks!

[dpankros]

The final decision of what the behavior of ambassador should be is, of course, up to your team. To me, however, I tend to think that anything that defies the user's expectations is a defect. It is either: 1) a defect of implementation (A doesn't behave as required), or 2) a defect of explanation (method A really does B).

Here, when one adds a header, I would expect the header to be set to that value, not that value plus something else, not a variant, but the value exactly as provided. Thus, that the resulting header value is "http, https" would be a defect by my standard because it is not literally "https", as provided. I would have expected to need to do something like:

add_request_headers:
  x-forwarded-proto: "%PROTOCOL%,https"

to get the behavior I am seeing (to concatenate what I _want_ with what I _have_). Instead, I'm forced into the current behavior (a comma-separated list) without having an option for the behavior I want (a single value) where other implementations could do either.

If, as you say, "Ambassador seems to be doing the right thing," maybe the naming is inaccurate because the behavior does not seem correct to me. If you discuss with your team at some point, please consider whether this is this how all the headers work, or if this is an undocumented special case, and whether this is the most flexible implementation for your users.

Thanks for your help, regardless.

[tkurylek]

I think this behaviour is now configurable as stated in documentation:

add_request_headers:
  x-forwarded-proto: 
    value: "https"
    append: False