fix(proxy): normalize trailing-dot CONNECT hosts before policy evaluation - #2248
fix(proxy): normalize trailing-dot CONNECT hosts before policy evaluation#2248letv1nnn wants to merge 2 commits into
Conversation
|
/ok to test 8384df5 |
8384df5 to
1544036
Compare
r3v5
left a comment
r3v5
left a comment
There was a problem hiding this comment.
PR: #2248
Author: letv1nnn (Artem Lytvyn)
Branch: 2201-normalize-trailing-dot-host -> main
Overview
Strips trailing dot from FQDN CONNECT hosts (e.g. api.example.com. → api.example.com) before policy evaluation, middleware selection, and credential matching. Fixes mismatches in K8s ndots:5 environments where dotted and undotted forms were treated as distinct hosts.
Key Design Decisions
- Normalize once, early (proxy.rs:613): Single normalize_connect_host() call right after parse_target(), so all downstream policy logic sees the clean hostname.
- Selective raw_host pass-through (proxy.rs:784, proxy.rs:837): resolve_and_check_trusted_gateway and resolve_and_check_allowed_ips still receive raw_host to preserve the ndots:5 DNS bypass
behavior.
Notable Code
crates/openshell-supervisor-network/src/proxy.rs:4546
fn normalize_connect_host(raw_host: &str) -> &str {
raw_host.strip_suffix('.').unwrap_or(raw_host)
}
Clean, zero-allocation — returns a sub-slice. Only strips one dot per RFC 1034.
1544036 to
0ae8014
Compare
|
/ok to test 0ae8014 |
|
Label |
|
Hosts need to be normalized in the forward-proxy path as well. Currently, non-CONNECT requests are dispatched prior to the normalization. I think if we duplicate the normalization for now that's ok. We're going to refactor the proxy and align CONNECT and non-CONNECT paths under the same code. But to merge this as-is we need to make sure we normalize for both pathways. |
|
Also need to normalize hostnames for hosts file ( |
0ae8014 to
6e09c21
Compare
|
This pull request has had no activity for 14 days and is now marked stale. It may be closed in 7 days if there is no further activity. |
github-actions
Bot
added
the
state:stale
6e09c21 to
fe03fb4
Compare
|
hey @johntmyers, thanks for picking this up. Anything left from my side, or ready to merge? |
|
/ok to test fe03fb4 |
…tion Signed-off-by: Artem Lytvyn <alytvyn@redhat.com>
fe03fb4 to
329dc44
Compare
johntmyers
left a comment
johntmyers
left a comment
There was a problem hiding this comment.
gator-agent
PR Review Status
Thanks @johntmyers. I checked the two paths you called out: the current patch now normalizes forward-proxy policy inputs and hosts-file lookup keys while retaining the dotted spelling for local DNS. One supported DNS sink still loses that spelling when CONNECT-by-hostname is enabled for an upstream proxy.
Action required: @letv1nnn, preserve raw_host for the upstream proxy's hostname CONNECT target and add the focused request-line regression test described inline.
Blocking findings:
GATOR-329dc440-01: the upstream-proxy hostname mode receives the normalized host instead of the absolute FQDN.
Carried findings:
- None
Gator metadata
- Validation: Project-valid localized fix for maintainer-authored issue #2201; verified maintainer feedback defines the forward-proxy and hosts-file requirements.
- Docs: Not needed because this restores existing valid-FQDN behavior without changing commands, configuration, policy syntax, or documented workflows.
- Checks: DCO is green; Branch Checks, Helm Lint, and E2E are pending for the current head.
- E2E:
test:e2eis applied; dispatch is deferred until the blocking code finding is resolved. - Head SHA:
329dc440656c07943d70e90c5f95e87060c316dd - Base SHA:
dc374e88784a7e4a7366abd1fdb56c969efe6769 - Merge base SHA:
dc374e88784a7e4a7366abd1fdb56c969efe6769 - Patch ID:
ab45a6fddae8af48ecab38756c6f6c586af02da5 - Gator payload:
7 - Review mode:
initial - Previous reviewed SHA: none
- Review budget exhausted: no
- Maintainer decision required: no
- Next state:
gator:in-review
johntmyers
added
the
gator:in-review
…host and add regression test Signed-off-by: Artem Lytvyn <alytvyn@redhat.com>
|
/ok to test 6f0ad2f |
johntmyers
left a comment
johntmyers
left a comment
There was a problem hiding this comment.
gator-agent
PR Review Status
The latest author commit resolves GATOR-329dc440-01: the normalized hostname remains in policy and routing decisions, while the raw dotted FQDN now reaches the upstream proxy's hostname CONNECT target. The focused request-line regression test covers the required behavior, and no blocking findings remain in the follow-up delta.
Blocking findings:
- No blocking findings remain
Carried findings:
- None;
GATOR-329dc440-01is resolved
Gator metadata
- Validation: Project-valid localized fix for maintainer-authored issue #2201; the latest delta addresses the prior supported-path obligation.
- Docs: Not needed because this restores existing valid-FQDN behavior without changing commands, configuration, policy syntax, or documented workflows.
- Checks: DCO is green; Branch Checks, Helm Lint, and E2E are pending for the current head.
- E2E:
test:e2eis applied and/ok to test 6f0ad2f6acdf15c261073cb2e82a50f08c3b0569was posted; current-head workflow dispatch is awaiting confirmation. - Head SHA:
6f0ad2f6acdf15c261073cb2e82a50f08c3b0569 - Base SHA:
dc374e88784a7e4a7366abd1fdb56c969efe6769 - Merge base SHA:
dc374e88784a7e4a7366abd1fdb56c969efe6769 - Patch ID:
91839f2e035313dea507bc7591d603fd3fdd2452 - Gator payload:
7 - Review mode:
follow_up - Previous reviewed SHA:
329dc440656c07943d70e90c5f95e87060c316dd - Review budget exhausted: no
- Maintainer decision required: no
- Next state:
gator:in-reviewpending confirmed current-head test dispatch
johntmyers
added
gator:watch-pipeline
5 participants
Summary
Fixes inconsistent handling of trailing-dot hostnames (e.g. api.example.com.) in the sandbox supervisor's CONNECT proxy. Such hosts are valid RFC 1034 FQDNs used in Kubernetes
ndots:5environments to bypass search-domain expansion, but were treated as distinct from their undotted equivalents across policy evaluation, middleware selection, and credential matching.Related Issue
#2201
Changes
normalize_connect_host(raw_host: &str) -> &str {...}Testing
mise run pre-commitpassesChecklist