Skip to content

Account for ASP.NET Core changes around proxy header handling #1525

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.

Sign up for GitHub

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

Jump to bottom
Open
wants to merge 4 commits into
base: main
Choose a base branch
from
Open

Account for ASP.NET Core changes around proxy header handling #1525

wants to merge 4 commits into from
+510 −7

Conversation

TimHess
Copy link
Member

@TimHess TimHess commented Jun 11, 2025 •
edited
Loading

Description

Provides a fix for #1524 that is automatically implemented for certificate auth

Quality checklist

  • Your code complies with our Coding Style.
  • You've updated unit and/or integration tests for your change, where applicable.
  • You've updated documentation for your change, where applicable.
    If your change affects other repositories, such as Documentation, Samples and/or MainSite, add linked PRs here.
  • There's an open issue for the PR that you are making. If you'd like to propose a new feature or change, please open an issue to discuss the change or find an existing issue.
  • You've added required license files and/or file headers (explaining where the code came from with proper attribution), where code is copied from StackOverflow, a blog, or OSS.

bart-vmware
bart-vmware reviewed Jun 12, 2025
View reviewed changes
Copy link
Member

@bart-vmware bart-vmware left a comment

Choose a reason for hiding this comment

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

Thanks for diving into this. I haven't tested the changes, assuming you did.

@bart-vmware bart-vmware added Component/Security Issues related to Steeltoe Security components (not app-sec) ReleaseLine/4.x Identified as a feature/fix for the 4.x release line labels Jun 12, 2025
@bart-vmware bart-vmware added this to the 4.0.0-rc1 milestone Jun 12, 2025
@TimHess
Copy link
Member Author

TimHess commented Jun 12, 2025

Thanks for diving into this. I haven't tested the changes, assuming you did.

Yes, but also because SteeltoeOSS/Samples#391 works with the same set of sample apps and I was hitting weird probably-environment issues during my first round of testing, I was already planning to leave this PR draft/open while wrapping up the other effort

@TimHess TimHess force-pushed the unknown_reverse_proxy branch 2 times, most recently from 9f1e858 to 020bf64 Compare June 12, 2025 19:35
@TimHess TimHess mentioned this pull request Jun 13, 2025
Open
@TimHess TimHess force-pushed the unknown_reverse_proxy branch 2 times, most recently from 80a4e65 to e4ad593 Compare June 13, 2025 20:19
@TimHess TimHess marked this pull request as ready for review June 13, 2025 20:42
@TimHess TimHess requested a review from bart-vmware June 13, 2025 21:15
bart-vmware
bart-vmware reviewed Jun 16, 2025
View reviewed changes
bart-vmware
bart-vmware reviewed Jun 16, 2025
View reviewed changes
bart-vmware
bart-vmware reviewed Jun 16, 2025
View reviewed changes
TimHess added 2 commits June 18, 2025 13:54
@TimHess
Improve CF reverse proxy support
507a8c2 
- Move reverse proxy configuration to Common
- When present, add networks for CF_INSTANCE IP vars to KnownNetworks
- Parameter-less UseCertificateAuthorization now tries to retrieve ForwardedHeadersOptions from DI container before falling back on creating a new instance
@TimHess
PR feedback
9c0da20 
- Move new ServiceCollectionExtension to Configuration.CloudFoundry
- Use IConfigureOptions<ForwardedHeadersOptions>
- add missing using on ServiceProviders, true in BuildServiceProvider
- more consistent usage of EnvironmentVariableScope
- remove CF_INSTANCE var parsing option
@TimHess TimHess force-pushed the unknown_reverse_proxy branch from e4ad593 to 9c0da20 Compare June 18, 2025 18:55
@TimHess TimHess changed the title When present, add CF_INSTANCE_INTERNAL_IP to ASP.NET Core Known Proxies Account for ASP.NET Core changes around proxy header handling Jun 18, 2025
@TimHess TimHess self-assigned this Jun 18, 2025
@TimHess TimHess requested a review from bart-vmware June 18, 2025 21:06
bart-vmware
bart-vmware reviewed Jun 19, 2025
View reviewed changes
src/Configuration/src/CloudFoundry/ForwardedHeadersSettings.cs
internal const string ConfigurationKey = "Steeltoe:ForwardedHeaders";

/// <summary>
/// Gets or sets a value indicating whether to trust all networks (adds 0.0.0.0/0). WARNING: Use only behind a trusted ingress.
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
/// Gets or sets a value indicating whether to trust all networks (adds 0.0.0.0/0). WARNING: Use only behind a trusted ingress.
/// Gets or sets a value indicating whether to trust all networks (adds 0.0.0.0/0). WARNING: Use only behind a trusted ingress. Default value: true.

src/Configuration/src/CloudFoundry/ForwardedHeadersSettings.cs
/// <summary>
/// Gets or sets known networks to trust, in CIDR notation. Example: "10.0.0.0/8,192.168.0.0/16".
/// </summary>
public string? KnownNetworks { get; set; }
Copy link
Member

Choose a reason for hiding this comment

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

According to https://nestenius.se/net/configuring-asp-net-core-forwarded-headers-middleware, KnownNetworks can already be specified in appsettings and is bound automatically (when using a non-empty host builder, see here and here). So I don't think we need to introduce our own option for that. Based on ASPNETCORE_FORWARDEDHEADERS_ENABLED, the middleware is added automatically using a startup filter (when using a non-empty host builder, it defaults to true).

I verified that the following appsettings.json properly binds in a sample:

"ForwardedHeaders": {
  "ForwardedHeaders": "XForwardedFor,XForwardedHost,XForwardedProto",
  "ForwardLimit": 1,
  "RequireHeaderSymmetry": true,
  "KnownProxies": [ "::1" ],
  "KnownNetworks": [ "127.0.0.1/8" ],
  "AllowedHosts": [ "nestenius.se" ]
}

image

src/Configuration/src/CloudFoundry/ForwardedHeadersSettings.cs

namespace Steeltoe.Configuration.CloudFoundry;

public sealed class ForwardedHeadersSettings
Copy link
Member

Choose a reason for hiding this comment

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

The name *Settings is misleading, because Aspire uses that to indicate it's not an IOptions.

If this type is going to stay, it should be added to the configuration schema.

src/Configuration/test/CloudFoundry.Test/CloudFoundryServiceCollectionExtensionsTest.cs
@@ -49,4 +54,285 @@ public async Task ConfigureCloudFoundryOptions_ConfiguresCloudFoundryOptions()
Assert.Equal(16384, appOptions.Value.Limits?.FileDescriptor);
Assert.Equal("playground", appOptions.Value.SpaceName);
}

Copy link
Member

Choose a reason for hiding this comment

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

We should have a test using TestWebApplicationBuilderFactory.CreateDefault() to verify settings are bound from configuration (using both our own configurer, and the one from ASP.NET), and that the startup filter adds the middleware.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@bart-vmware bart-vmware bart-vmware left review comments

At least 1 approving review is required to merge this pull request.

Assignees

@TimHess TimHess

Labels
Component/Security Issues related to Steeltoe Security components (not app-sec) ReleaseLine/4.x Identified as a feature/fix for the 4.x release line
Projects
None yet
Milestone
4.0.0-rc1
Development

Successfully merging this pull request may close these issues.
2 participants
@TimHess @bart-vmware