#125 refuse the empty hostname in the pattern matcher - #132
Merged
Merged
Conversation
strings.Split("", ".") yields one empty label, which both `*` and `**`
consumed, so an IP with no reverse name matched any wildcard rule.
gateExistingConnections then opened it on the rule's ports instead of
its observed ports (dropping the in-flight flow mid-stream a minute
later) and pinned L7 scope to an empty name.
matchSegments now leaves an empty label unmatched: "" is never a DNS
label, so no segment may consume it. That makes "" match nothing at
every entry point (MatchHostnameRule, FindTrackedHostname,
GetAutoAllowedTypeForHostname) and also stops `**` from bridging a
stray empty label ("a..b", "github.com.").
Signed-off-by: Matthew DeVenny <matt@codecargo.com>
There was a problem hiding this comment.
Pull request overview
Prevents wildcard hostname rules from matching empty or unresolvable hostnames, preserving observed-port handling for existing connections.
Changes:
- Rejects empty labels during hostname matching.
- Adds matcher and configuration regression tests.
- Adds an existing-connection gating test.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
pkg/config/pattern.go |
Rejects empty hostname labels. |
pkg/config/pattern_test.go |
Tests wildcard and empty-label behavior. |
pkg/config/config_test.go |
Tests configuration matching and attribution. |
cmd/start_test.go |
Tests observed-port preservation for unresolvable IPs. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Review: the empty-label refusal belongs at the edge, the same way compileHostnamePattern rejects empty segments, so matchSegments stays a plain glob over well-formed labels. Matches now returns false for any hostname with an empty label; the DP loses its special case and keeps only the loop-bound cleanup. The appended MatchHostnameRule test asserted two APIs that cannot observe the bug (FindTrackedHostname attributes patterns to the query name, so "" in is "" out either way; GetAutoAllowedTypeForHostname skips every rule LoadConfigFromRules produces). Replaced by one query-"" row in TestMatchHostnameRule_Table. Signed-off-by: Matthew DeVenny <matt@codecargo.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Closes #125.
Symptom
A pre-existing connection whose IP has no reverse-DNS name is meant to be kept alive on the ports it was observed using. With a
*or**hostname rule anywhere in the policy it took that rule's ports instead, so a flow on 443 was allowed on 123 and dropped mid-stream a minute later. The same branch pinned L7 scope to an empty hostname.Cause
gateExistingConnectionspasses""(the no-reverse-name result) toMatchHostnameRule.strings.Split("", ".")yields one empty label, and both wildcard forms happily consume one label, soverdict.HasAllow()was true and the observed-ports carve-out never ran.Fix
hostnamePattern.Matchesnow returns false for any hostname with an empty label, at the boundary, the same waycompileHostnamePatternrejects empty segments.""is never a DNS label, so no segment may consume it;matchSegmentsstays a plain glob over well-formed labels and only picks up a loop-bound cleanup. This is done in the matcher rather than at the call site becauseMatchHostnameRule("")is reachable from other paths (a root DNS query trims to""too) and""is never a real name.Tests
TestHostnamePatternMatches:*,**,*.x,**.x, and a literal all reject"";**rejects interior and trailing empty labels.TestMatchHostnameRule_Table: aquery: ""row against a bare**allow returns the empty verdict.TestGateExistingConnections_UnresolvableIgnoresWildcardRule: the issue's exact scenario. A**allow on 123 plus an unresolvable IP observed on 443 yields an allow on 443 only and no L7 scope.The table row, the wildcard pattern cases, and the gate test all fail against main's matcher. Full
go test ./...is green in the Lima VM on the CI kernel;gofumpt,vet, andstaticcheckare clean.