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.

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

Set internal_address_config on HttpConnectionManager explicitly #12190

Closed
jijiechen opened this issue Dec 9, 2024 · 7 comments · Fixed by #12986
Closed

Set internal_address_config on HttpConnectionManager explicitly #12190

jijiechen opened this issue Dec 9, 2024 · 7 comments · Fixed by #12986
Assignees
@jijiechen
Labels
kind/feature New feature triage/accepted The issue was reviewed and is complete enough to start working on it
Milestone
2.10.x

Comments

@jijiechen
Copy link
Member

jijiechen commented Dec 9, 2024 •
edited
Loading

Description

Envoy is warning when HCM does not have explicit internal_address_config set and it will not consider any IP address as internal in a future release, so we need to adapte to this change before our next Envoy version bump.

This is considered a feature because it did not impact our existing versions.

Reproduce steps

  1. download Kuma
  2. Run the zone cp and a DP following the quick start guide
  3. check the sidecar log

Actual result

A warning shows up:

[2024-12-09 06:12:49.716][20][warning][misc] [source/extensions/filters/network/http_connection_manager/config.cc:82] internal_address_config is not configured. The existing default behaviour will trust RFC1918 IP addresses, but this will be changed in next release. Please explictily config internal address config as the migration step.

Expected result

No warning.

More information

As the warning states, the HCM recognized IP addresses defined in RFC1918 as internal by default, and this will be changed in a newer release. This will impact the behaviour of logging and header santination of Envoy.

This behaviour can be controlled by Envoy runtime feature flag
envoy.reloadable_features.explicit_internal_address_config
This feature flag defaults to false in all existing Envoy versions (1.29.11, 1.32.2), but the code has been changed to defaults to true on the main branch.

There is a related Envoy CVE:

  • CVE-2024-45806: (CVSS Score 6.5, Moderate): Potential for x-envoy headers to be manipulated by external sources.

What is an internal address?

.InternalAddressConfig Configures what network addresses are considered internal for stats and header sanitation purposes. If unspecified, only RFC1918 IP addresses will be considered internal. See the documentation for x-envoy-internal for more information about internal/external addresses.
Source: Envoy docs

Other projects had fixed/discussed simimar issue:

Envoy implementation

The runtime feature flag: https://github.com/envoyproxy/envoy/blob/a0504e87c5a246cb097b37049b1e4dc7706c2a90/source/common/http/conn_manager_config.h#L194

Default implementation: https://github.com/envoyproxy/envoy/blob/v1.32.2/source/common/network/utility.cc#L272
@jijiechen jijiechen added triage/pending This issue will be looked at on the next triage meeting kind/feature New feature labels Dec 9, 2024
@lukidzi lukidzi added this to the 2.10.x milestone Dec 9, 2024
@lukidzi
Copy link
Contributor

lukidzi commented Dec 9, 2024 •
edited
Loading

Triage: We should configure defaults compliant with RFC. The question is if there are cases of using a different pool of addresses than one in RFC? We should check if that is the case.

@lukidzi lukidzi added triage/accepted The issue was reviewed and is complete enough to start working on it and removed triage/pending This issue will be looked at on the next triage meeting labels Dec 9, 2024
@jijiechen
Copy link
Member Author

This configuration can be a zone CP configuration since it can differ from zone to zone.

@jijiechen jijiechen self-assigned this Feb 25, 2025
@jijiechen
Copy link
Member Author

The question is if there are cases of using a different pool of addresses than one in RFC? We should check if that is the case.

Larger enterprises have clusters with complicated networking. Some of them have very customized IP address pool. It's quite possible that they have multiple internal clusters that have different IP CIDRs and not compliant to the RFCs.
For example, I worked for a company, their were using 9.0.0.0/8 and 11.0.0.0/8.

@lahabana
Copy link
Contributor

Sounds like we need a new CP configuration to add the internal address pool.
A good default value should be the one that was used by default in Envoy before this became a required setting.

@lahabana
Copy link
Contributor

Adding this setting as a value in Proxy is likely the best.

This can be passed from the settings in there:

kuma/pkg/xds/sync/components.go

Line 18 in 3ea4292

return &DataplaneProxyBuilder{

@jijiechen jijiechen mentioned this issue Mar 3, 2025
Merged
@jijiechen
Copy link
Member Author

We are using Envoy 1.32 in 2.10, and Envoy 1.32 removed the default builtin internal addresses. So it is better to implement this one in our 2.10 release.
Source: https://github.com/envoyproxy/envoy/blob/main/source/extensions/filters/network/http_connection_manager/config.cc#L83-L86

@jijiechen
Copy link
Member Author

The isInternalRequest is used to check if a request is originated from an internal client, so it's not necessary to include these Mesh*Service CIDRs since they are only used as destinations.
https://github.com/envoyproxy/envoy/blob/main/source/common/http/conn_manager_utility.cc#L222-L251

Envoy attaches a set of request headers when forwarding requests to the upstream. It removes "internal only headers" from requests originated from external clients, so that, in this case, the upstream server only receives trusted/sanitized headers.
https://github.com/envoyproxy/envoy/blob/main/source/common/http/conn_manager_utility.cc#L319

jijiechen added a commit that referenced this issue Mar 12, 2025
@jijiechen
feat(xds): add internal address config onto HttpConnectionManager (#1…
8b33058 
…2986)

## Motivation

add internal address config onto HttpConnectionManager, more details are
available in issue #12190

## Implementation information

introducing a new configuration item `ipam.knownInternalCIDRs` on zonal
CP to allow users specify their known internal address pool and we
assign these values when generating Envoy config for
HttpConnectionManager

## Supporting documentation

<!-- Is there a MADR? An Issue? A related PR? -->

fixes #12190

<!--
> Changelog: skip
-->
<!--
Uncomment the above section to explicitly set a [`> Changelog:` entry
here](https://github.com/kumahq/kuma/blob/master/CONTRIBUTING.md#submitting-a-patch)?
-->

---------

Signed-off-by: Jay Chen <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@jijiechen jijiechen

Labels
kind/feature New feature triage/accepted The issue was reviewed and is complete enough to start working on it
Projects
None yet
Milestone
2.10.x
Development

Successfully merging a pull request may close this issue.

feat(xds): add internal address config onto HttpConnectionManager jijiechen/kuma
3 participants
@lahabana @jijiechen @lukidzi