Skip to content

Conversation

Tratcher
Copy link
Member

@Tratcher Tratcher commented Jul 2, 2021

Fixes #1114. YARP copies request and response headers by default. A security conscious admin may want to only allow specific headers in or out. This PR implements allow lists for request headers, response headers and response trailers as per route transforms.

I also backfilled some tests for the Request/Response/Trailers Remove transforms and fixed a bug with ResponseHeaderRemove using the wrong key. (Fixes #1141)

@Tratcher Tratcher added this to the YARP RC1 milestone Jul 2, 2021
@Tratcher Tratcher requested review from samsp-msft and alnikola July 2, 2021 22:18
@Tratcher Tratcher self-assigned this Jul 2, 2021
@Tratcher Tratcher requested a review from MihaZupan as a code owner July 2, 2021 22:18
Copy link
Member

@MihaZupan MihaZupan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am not as familiar with transforms, but nothing stands out to me.

I take it the user is resonsible for providing an exact list (i.e. we're not adding any common header exceptions aside from Hsot)?

@Tratcher
Copy link
Member Author

Tratcher commented Jul 7, 2021

Yes, the user must provide an exact list. Not even Host is exempted here, if you don't allow the host header then the original won't be copied. I think the only exception is that WebSocket/Upgrade headers will be re-added later:
https://github.com/microsoft/reverse-proxy/blob/56b8e7bc5baa060061521c1d8875c94e0f3f7ecc/src/ReverseProxy/Forwarder/HttpForwarder.cs#L315-L333

Copy link
Contributor

@alnikola alnikola left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@Tratcher Tratcher merged commit bfb6406 into main Jul 14, 2021
@Tratcher Tratcher deleted the tratcher/allowed branch July 14, 2021 18:32
@samsp-msft
Copy link
Contributor

It sounds then like this is applied after other transforms, so if people are modifying headers with code, will those need to be on this list?

@Tratcher
Copy link
Member Author

Transforms are applied in the order they're specified in config. This also only affects the copying of headers from HttpContext.Request to HttpRequestMessage or from HttpResponseMessage to HttpContext.Response. If you're modifying HttpContext and want that copied over to HttpRequestMessage then yes it needs to be listed. If you're directly modifying HttpRequestMessage then it doesn't.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Unknown transform: ResponseHeaderRemove YARP should have an option for an allow list of headers
4 participants