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

Updated Dashboard to accept/map forwarded headers #6969

Open
wants to merge 6 commits into
base: main
Choose a base branch
from

Conversation

danespinosa
Copy link

@danespinosa danespinosa commented Dec 19, 2024

Description

This change addresses the need to map the forwarded headers in the application when ASPNETCORE_FORWARDEDHEADERS_ENABLED is enabled. Currently even if a developer enables the ASPNETCORE_FORWARDEDHEADERS_ENABLED environment variable, the application does not map the forwarded headers properly because the application doesn't specify which headers to map.

Without this change the dashboard doesn't work well behind a reverse proxy like YARP when doing OpenID Auth since the app doesn't map the Host or Protocol (http/https) properly and the redirect ends up being the address YARP redirects the call to.

I need guidance for the test I was thinking calling the dashboard and do something similar to ValidateTokenMiddlewareTests to validate the Host, and Proto.

Fixes # (issue)

Checklist

  • Is this feature complete?
    • Yes. Ready to ship.
    • No. Follow-up changes expected.
  • Are you including unit tests for the changes and scenario tests if relevant?
    • Yes
    • No
  • Did you add public API?
    • Yes
      • If yes, did you have an API Review for it?
        • Yes
        • No
      • Did you add <remarks /> and <code /> elements on your triple slash comments?
        • Yes
        • No
    • No
  • Does the change make any security assumptions or guarantees?
    • Yes, it assumes the developer knows that forwarding headers and mapping them is required for the dashboard to work properly when behind a reverse proxy like YARP. This in certain scenarios could pose a security risk, although it's documented that this dashboard shouldn't be used in production environments.
      • If yes, have you done a threat model and had a security review?
        • Yes
        • No
    • No
  • Does the change require an update in our Aspire docs?
    • Yes
      • Is this introducing a breaking change?
        • Yes
        • No, if this change is accepted it would be good to clarify that the only 2 forwarded headers allowed are the Host and Protocol in here Configuration
    • No
Microsoft Reviewers: Open in CodeFlow

@dotnet-policy-service dotnet-policy-service bot added the community-contribution Indicates that the PR has been added by a community member label Dec 19, 2024
@adamint
Copy link
Member

adamint commented Dec 19, 2024

@JamesNK @drewnoakes
I'm not sure whether there are any security implications here.

@davidfowl
Copy link
Member

I’m that doesn’t seem right. Do you need to forward all headers?

@danespinosa
Copy link
Author

danespinosa commented Dec 19, 2024

I’m that doesn’t seem right. Do you need to forward all headers?

@davidfowl I just need XForwardedHost/X-Forwarded-Host that maps to host, that unblocks me to use Microsoft as an Identity provider, I wouldn't know if we need any other headers for other OpenId identity provider though, the other option would be to expose the ForwardedHeaderOptions via the config and let the users decide what headers to forward?

For example if a developer uses Yarp to cut the SSL connection at the edge they will need to map the Protocol too.

@danespinosa
Copy link
Author

I’m that doesn’t seem right. Do you need to forward all headers?

@adamint @davidfowl i have limited the forwarded headers to host and protocol, let me know your thoughts.

@danespinosa danespinosa changed the title updated to accept forwarded headers Updated Dashboard to accept/map forwarded headers Dec 24, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
community-contribution Indicates that the PR has been added by a community member
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants