Add inbound support for svc hostname in authority #1420

jaellio · 2025-01-10T00:18:28Z

Hoping to get feedback on the overall approach for this change. I would appreciate any rust specific feedback as well.

PR still needs some minor refactoring.

Main changes:

hbone_address supports hostname or ip when reading from the authority header
when a hostname is included in the authority pseudo header in is included as header in the proxy protocol. The hbone address included in the address header is set to the destination service vip
hostname is validated by looking up the respective service and verifying the destination pod ip is a member of that service.

Todo:

refactor hbone address validation to improve testability
update error types
confirm if port should also be included with hostname in custom protocol type
intelligently select service vip for proxy protocol address header
strictly validate hostname fqdn

istio-testing · 2025-01-10T00:18:31Z

Skipping CI for Draft Pull Request.
If you want CI signal for your change, please convert it to an actual PR.
You can still manually trigger a test run with /test all

istio-policy-bot · 2025-01-10T00:18:32Z

😊 Welcome @jaellio! This is either your first contribution to the Istio ztunnel repo, or it's been
a while since you've been here.

You can learn more about the Istio working groups, Code of Conduct, and contribution guidelines
by referring to Contributing to Istio.

Thanks for contributing!

Courtesy of your friendly welcome wagon.

jaellio · 2025-01-10T00:18:53Z

/test all

jaellio · 2025-01-10T00:20:09Z

src/proxy/inbound.rs

+            // TODO(jaellio): Should we also validate the hbone_addr port?
+            HboneAddress::SvcHostname(hbone_host, hbone_port) => {
+                // Validate a service or workload exists for the hostname
+                let parts: Vec<&str> = hbone_host.split('.').collect();


TODO - figure out if there is a better way to parse a hostname

I don't think we can. There is not really a mapping from hostname -> namespace. For example, I could have a hostname foo.com. We could have a guess or heuristic, but this doesn't seem like a time when doing the check only some of the time provides much value?

Since its to look up the service, perhaps we do something like find all the services with the hostname and pick the first preferring one with the same namespace (as the workload) if available

Took a shot at this with ranking - might be a simpler way to do it in rust but I am not at all familiar :) Let me know if it's similar to what you had in mind.

jaellio · 2025-01-10T00:42:04Z

/test test

jaellio · 2025-01-10T00:56:26Z

/test test

src/proxy/inbound.rs

keithmattix · 2025-01-10T21:40:23Z

src/proxy/inbound.rs


        // Determine the next hop.
+        let hbone_addr_clone = hbone_addr.clone();


Making hbone_addr an Arc may help with the clones

I think it could probably be changed so that find_inbound_upstream takes a borrow. Not sure why it accepts so many borrows and one owned TBH. All we really care about here is the port I think which is cheap to copy on the stack vs a clone of something more complex

keithmattix · 2025-01-10T21:42:11Z

src/proxy/inbound.rs

+            HboneAddress::SvcHostname(hbone_host, hbone_port) => {
+                // Validate a service or workload exists for the hostname
+                let parts: Vec<&str> = hbone_host.split('.').collect();
+                if parts.len() <= 1 {


We can probably be a little stronger here and require an FQDN (i.e. error if we detect a short name with less than 5 parts). I think Istio always requires FQDNs for multi-cluster addressing (/cc @howardjohn)

Agree. Kept it simple for the draft but this isn't sufficient atm.

Yes to require FQDN. However I am not convinced we can/should do the mapping of service --> namespace at all (see comment 2 lines up)

src/proxy/inbound.rs

keithmattix · 2025-01-10T21:46:05Z

src/proxy/inbound.rs

-        hbone_addr: SocketAddr,
-    ) -> Result<(), Error> {
+        hbone_addr: HboneAddress,
+    ) -> Result<Option<SocketAddr>, Error> {


Don't love the idea of returning a value from this function. I know you need it for the proxy protocol stuff so maybe the initial hbone_addr matching can be a part of a separate function that's called only if the :authority is a hostname

Agree, just a temporary hack. Will fix

jaellio · 2025-01-15T05:10:16Z

src/proxy/inbound.rs

+                                StatusCode::BAD_REQUEST,
+                            ));
+                        }
+                        // TODO(jaellio): Intelligently select the VIP


@keithmattix with regards to #1409 and intelligently selecting vip for a Address::Service, is it sufficient to rank vips by network comparison (higher rank if vip.network matches conn.dst_network) or should we also be comparing IpAddr ranges? Do we also want to implement ranking of Address::Services when finding hostnames (similar to how there would be duplicate workloads on lookup by IP)?

Sorry, missed this. Are you still blocked here?

Based on this comment we can't rely on the hostname to include the namespace. So right now we'll need to change some of the logic for finding a service by hostname and implement a ranking as suggested here

Not blocked, I'll implement the service ranking and vip ranking as suggested above. What just curious if you had any thoughts on the approach.

@keithmattix Implemented simple ranking for service (when searching without a namespace) and vip. There might be a simpler way to accomplish the ranking but took inspiration from #1409

howardjohn

A

howardjohn · 2025-01-15T22:45:06Z

src/proxy.rs

 const PROXY_PROTOCOL_AUTHORITY_TLV: u8 = 0xD0;
+// Custom TLV for including the svc hostname in the proxy protocol header
+const PROXY_PROTOCOL_SVC_HOSTNAME_TLV: u8 = 0xD1;


Hmm this is an interesting one. I don't see any strong reason not to do this other than I don't know of any use cases to do so, and if we do we probably can never remove it since its part of our runtime API contract.

Did you have a specific use in mind for this?

I mean, I imagine that if there's custom waypoint sandwiched with the ztunnel, it would want to use the hostname for routing (vs. an IP address we lookup) , right?

@keithmattix correct me if I am wrong here. We were concerned about not preserving the hostname in the proxy protocol. Not positive about the exact implications of this for logging, but maybe it has more implications for security tools?

howardjohn · 2025-01-15T22:56:30Z

src/proxy/inbound.rs

@@ -262,38 +346,90 @@ impl Inbound {
        }

        let start = Instant::now();
+
+        // Extract the host or IP from the authority pseudo-header of the URI
        let hbone_addr = req


nit: you could make this a

impl TryFrom<&http::Uri> for HboneAddress { type Error = Error; fn try_from(value: &http::Uri) -> Result<Self, Self::Error> { ... }

Then the calling code is just req.uri().try_into()

Took a shot at it, let me know if it's what you had in mind

howardjohn · 2025-01-15T22:57:59Z

src/proxy/inbound.rs

+                let hbone_host = req.uri().host().unwrap();
+                let hbone_port = req.uri().port_u16().unwrap();


I don't think these unwraps are safe. I suspect :authority: foo (no port) would panic here for example. Which we want to reject (probably with a BAD_REQUEST), but shouldn't unwrap.

Moved this logic to the TryFrom implementation for HboneAddress. Returns an error result if port or host are not set.

src/proxy/inbound.rs

howardjohn · 2025-01-15T23:10:21Z

src/proxy/inbound.rs

+            // TODO(jaellio): Should we also validate the hbone_addr port?
+            HboneAddress::SvcHostname(hbone_host, hbone_port) => {
+                // Validate a service or workload exists for the hostname
+                let parts: Vec<&str> = hbone_host.split('.').collect();


I don't think we can. There is not really a mapping from hostname -> namespace. For example, I could have a hostname foo.com. We could have a guess or heuristic, but this doesn't seem like a time when doing the check only some of the time provides much value?

howardjohn · 2025-01-15T23:11:09Z

src/proxy/inbound.rs

+            HboneAddress::SvcHostname(hbone_host, hbone_port) => {
+                // Validate a service or workload exists for the hostname
+                let parts: Vec<&str> = hbone_host.split('.').collect();
+                if parts.len() <= 1 {


Yes to require FQDN. However I am not convinced we can/should do the mapping of service --> namespace at all (see comment 2 lines up)

howardjohn · 2025-01-15T23:12:06Z

src/proxy/inbound.rs

+            // TODO(jaellio): Should we also validate the hbone_addr port?
+            HboneAddress::SvcHostname(hbone_host, hbone_port) => {
+                // Validate a service or workload exists for the hostname
+                let parts: Vec<&str> = hbone_host.split('.').collect();


Since its to look up the service, perhaps we do something like find all the services with the hostname and pick the first preferring one with the same namespace (as the workload) if available

src/proxy/inbound.rs

Signed-off-by: Jackie Elliott <[email protected]>

Implement TryFrom from uri to hboneAddress Signed-off-by: Jackie Elliott <[email protected]>

istio-testing added the do-not-merge/work-in-progress Block merging of a PR because it isn't ready yet. label Jan 10, 2025

istio-testing added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Jan 10, 2025

jaellio commented Jan 10, 2025

View reviewed changes

Stevenjin8 reviewed Jan 10, 2025

View reviewed changes

src/proxy/inbound.rs Outdated Show resolved Hide resolved

keithmattix reviewed Jan 10, 2025

View reviewed changes

keithmattix linked an issue Jan 14, 2025 that may be closed by this pull request

Add service hostname HBONE :authority support to ztunnel servers #1422

Open

jaellio marked this pull request as ready for review January 14, 2025 23:28

jaellio requested a review from a team as a code owner January 14, 2025 23:28

istio-testing removed the do-not-merge/work-in-progress Block merging of a PR because it isn't ready yet. label Jan 14, 2025

jaellio commented Jan 15, 2025

View reviewed changes

jaellio added the do-not-merge/hold Block automatic merging of a PR. label Jan 15, 2025

howardjohn reviewed Jan 16, 2025

View reviewed changes

istio-testing added the needs-rebase Indicates a PR needs to be rebased before being merged label Jan 22, 2025

jaellio added 12 commits January 22, 2025 23:31

Support hostname in authority header

956a432

Signed-off-by: Jackie Elliott <[email protected]>

remove todos

cadae80

Signed-off-by: Jackie Elliott <[email protected]>

Use enum for host addr

e4f0b6b

Signed-off-by: Jackie Elliott <[email protected]>

Add HboneAddress enum and fix tests

61730d6

Signed-off-by: Jackie Elliott <[email protected]>

Update String usage to Strng and add comments

69f453e

Signed-off-by: Jackie Elliott <[email protected]>

Update svc_hostname to return strng

751244d

Signed-off-by: Jackie Elliott <[email protected]>

Fix build errors

d717b2a

Signed-off-by: Jackie Elliott <[email protected]>

Replace assert with panic for test configuration failure

bc9f0b6

Signed-off-by: Jackie Elliott <[email protected]>

Fix fmt errors

bb6a50a

Signed-off-by: Jackie Elliott <[email protected]>

Respond to PR feedback

c70bb3b

Signed-off-by: Jackie Elliott <[email protected]>

Respond to some PR feedback

920a9f6

Signed-off-by: Jackie Elliott <[email protected]>

Move HboneAddress to proxy

5a9a0b7

Signed-off-by: Jackie Elliott <[email protected]>

jaellio force-pushed the jaellio/rcvsvchostname branch from cf45ea3 to 5a9a0b7 Compare January 22, 2025 23:33

istio-testing removed the needs-rebase Indicates a PR needs to be rebased before being merged label Jan 22, 2025

Intelligently select service from hostname and vip from service.

3adcffb

Implement TryFrom from uri to hboneAddress Signed-off-by: Jackie Elliott <[email protected]>

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add inbound support for svc hostname in authority #1420

Add inbound support for svc hostname in authority #1420

jaellio commented Jan 10, 2025 •

edited

Loading

istio-testing commented Jan 10, 2025

istio-policy-bot commented Jan 10, 2025

jaellio commented Jan 10, 2025

jaellio Jan 10, 2025

howardjohn Jan 15, 2025

howardjohn Jan 15, 2025

jaellio Jan 23, 2025

jaellio commented Jan 10, 2025

jaellio commented Jan 10, 2025

keithmattix Jan 10, 2025

ilrudie Jan 15, 2025 •

edited

Loading

keithmattix Jan 10, 2025

jaellio Jan 11, 2025

howardjohn Jan 15, 2025

keithmattix Jan 10, 2025

jaellio Jan 11, 2025

jaellio Jan 15, 2025

keithmattix Jan 22, 2025

jaellio Jan 22, 2025

jaellio Jan 22, 2025

jaellio Jan 23, 2025

howardjohn left a comment

howardjohn Jan 15, 2025

keithmattix Jan 22, 2025

jaellio Jan 22, 2025

howardjohn Jan 15, 2025

jaellio Jan 23, 2025

howardjohn Jan 15, 2025

jaellio Jan 23, 2025

howardjohn Jan 15, 2025

howardjohn Jan 15, 2025

howardjohn Jan 15, 2025


		// Determine the next hop.
		let hbone_addr_clone = hbone_addr.clone();

		let hbone_host = req.uri().host().unwrap();
		let hbone_port = req.uri().port_u16().unwrap();

Add inbound support for svc hostname in authority #1420

Are you sure you want to change the base?

Add inbound support for svc hostname in authority #1420

Conversation

jaellio commented Jan 10, 2025 • edited Loading

istio-testing commented Jan 10, 2025

istio-policy-bot commented Jan 10, 2025

jaellio commented Jan 10, 2025

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jaellio commented Jan 10, 2025

jaellio commented Jan 10, 2025

Choose a reason for hiding this comment

ilrudie Jan 15, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

howardjohn left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jaellio commented Jan 10, 2025 •

edited

Loading

ilrudie Jan 15, 2025 •

edited

Loading