Ability to append a value to a set of headers

edubonifs commented 1 year ago

Gloo Edge Product

Enterprise

Gloo Edge Version

1.14.x

Kubernetes Version

1.25

Describe the bug

According to the docs, if I set append: true in a HeaderManipulation:

if the header HEADER_NAME is already present, append the value

Expected Behavior

What I understand with this, is that if I have the following headerManipulation in place:

      options:
        headerManipulation:
          responseHeadersToAdd:
          - header:
              key: x-version
              value: fake version
            append: true

And I receive a request with the responseHeader x-version: 5, the expected outcome would be that the value get happened to the existing header:

x-version: 5 fake version

However, what is happening, is that a new header with the same name gets created:

x-version: 5
x-version: fake version

We have a client that wants to append a string to a set of "set-cookie" headers, so if I have two response headers set-cookie, and I want to append a string, for example "; Partitioned", I should be able to do that: Response headers before HeaderManipulation:

set-cookie: TOKEN=eyJhbGciO...8; Version=1; Path=/blah/blah/; Secure; HttpOnly
set-cookie: AUTH_SESSION_ID=ec90fdc7-6f16-4ab1-bdfa-6a2478a1f25b; Version=1

Response headers after headerManipulation:

set-cookie: TOKEN=eyJhbGciO...8; Version=1; Path=/blah/blah/; Secure; HttpOnly; Partitioned
set-cookie: AUTH_SESSION_ID=ec90fdc7-6f16-4ab1-bdfa-6a2478a1f25b; Version=1; Partitioned

However, what I am getting is the following:

set-cookie: TOKEN=eyJhbGciO...8; Version=1; Path=/blah/blah/; Secure; HttpOnly
set-cookie: AUTH_SESSION_ID=ec90fdc7-6f16-4ab1-bdfa-6a2478a1f25b; Version=1; Path=/blah/blah/; SameSite=None; Secure; HttpOnly
set-cookie: ; Partitioned

Steps to reproduce the bug

Create a VS with the following headerManipulation:

      options:
        headerManipulation:
          responseHeadersToAdd:
          - header:
              key: x-version
              value: fake version
            append: true

Check that a new response header is created instead of being appended

Additional Environment Detail

No response

Additional Context

┆Issue is synchronized with this Asana task by Unito

inFocus7 commented 1 year ago

Haven't delved into this specific issue, but from my work on the append field deprecation issue, it looks like it won't address this.

From my local testing which uses Envoy's append_action, I see that with the following setup I still get multiple headers for the responseHeaderToAdd. For requestHeaderToAdd it looks like it acts as expected(?)

RouteTable

[...]
      headerManipulation:
        requestHeadersToAdd:
        - append: true
          header:
            key: test-request-header
            value: b
        responseHeadersToAdd:
        - append: false
          header:
            key: X-Frame-Options
            value: RTORIGIN
[...]

VirtualService

[...]
      headerManipulation:
        requestHeadersToAdd:
        - append: true
          header:
            key: test-request-header
            value: a
        responseHeadersToAdd:
        - append: true
          header:
            key: X-Frame-Options
            value: VSORIGIN
[...]

Result:

[...]
< x-frame-options: RTORIGIN
< x-frame-options: VSORIGIN
< 
{
  [...]
  "headers": {
    [...]
    "Test-Request-Header": "b,a", 
    [...]
  }, 
  [...]
}
[...]

edubonifs commented 1 year ago

Hi @inFocus7 ,

Do you think we can implement this behavior with any other envoy field that we could expose with Gloo Edge?

As it seems we cannot have this working with the current exposed fields

pandino commented 1 year ago

Hi @inFocus7, @edubonifs pointed me to this ticket. We also have a similar problem, however our case is a little more complex than the reported one: on top of adding lines to the set-cookie header we also need some logic attached. For example we would like to add the SameSite=Strict attribute only if the SameSite is not already defined. This is a simplified version of what we are currently doing:

[...]
          transformationTemplate:
            extractors:        
              secure:
                header: "set-cookie"
                regex: '^(.*[Ss]ecure)(.*)$'
              sameSite:
                header: "set-cookie"
                regex: '^(.*[Ss]ame[Ss]ite=)(.*)$'
            headersToAppend:        
              - key: set-cookie
                value:
                  text: "{% if header(\"set-cookie\") != \"\" %}{{ header(\"set-cookie\") }}{% if default(secure, \"\") == \"\" %}; Secure{% endif %}{% if default(sameSite, \"\") == \"\" %}; SameSite=Strict{% endif %}{% endif %}"
[...]

Our main problem here is that this works only for one header, there is no looping among multiple set-cookie headers.

I think the append action is meant to be used to append a new header to an existing header with the same name, folding the two together. In your test above, the request headers has been properly folded by combining the two header values in the same header name with a comma. So this may be not intended to add arbitrary strings to an header.

I do believe that arbitrary headers manipulation can be done in Envoy with a Lua script, but I can't see any way to pass a Lua filter in Gloo to the proxy. Probably this can be also done with a WASM filter, but again I don't understand if the filter can be applied to single routes (instead of adding it globally to the gateway proxy linked in the example) and the solution seems a little bit overcomplicated, aside not being ready for production....

edubonifs commented 8 months ago

Hi, another customer is now hitting this issue with jwt claimToHeader:

     jwtStaged:
        beforeExtAuth:
          providers:
            idp:
              keepToken: true
              claimsToHeaders:
                - claim: claim1
                  header: options
                - claim: claim2
                  header: options
                  append: true  
                - claim: claim3
                  header: options
                  append: true

Where the application is receiving the headers, not appended, but several headers with the same name and different values:

"options": "apple, banana"
"options": "orange"
"options": "kumquat"

If the issue is solved it will also help this customer

edubonifs commented 8 months ago

It would be good if we could control both behaviors:

if we want all the values to be on the same header
if we want to have different headers named the same with different values So users can choose between them

ShaunPTK commented 8 months ago

FYI - In the case mentioned above, where we are seeing:

"options": "apple, banana"
"options": "orange"
"options": "kumquat"

What our upstream is expecting is actually one of either:

"options": "apple"
"options": "banana"
"options": "orange"
"options": "kumquat"

or

"options" : "apple, banana, orange, kumquat"

The problem stems from the fact that we have a header being created from a JWT claim that already contains a comma separated list. The HTTP standard that relates to this type of header (https://www.rfc-editor.org/rfc/rfc9110.html#section-5.3) states that multiple headers with the same name MAY be treated as if they were a single comma separated string, but does not mention what might happen if, as we have here, the header is treated as a set/array but one of the items is already a string containing a list.

I think adding an option to concatenate to an existing header, and then another option to create a comma-separated list is overkill for what is an obscure issue like this. My feeling is that because our specification allows for one of these claims to be a list, then our upstream component should take the approach that it is explicitly ingesting the header as a string and then split to an array, which avoids this misinterpretation.

I was just following every possibility as far as this is concerned and it's now clear to me that our specific problem is as a result of one of our claims containing a list.

github-actions[bot] commented 2 months ago

This issue has been marked as stale because of no activity in the last 180 days. It will be closed in the next 180 days unless it is tagged "no stalebot" or other activity occurs.

anessi commented 2 months ago

This is still an issue for us, see https://github.com/solo-io/gloo/issues/8634#issuecomment-1727674731 So please keep it open, thanks.

solo-io / gloo