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.

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

Support DNS matchPattern="*" to match "." #11633

Merged
merged 1 commit into from May 21, 2020

Conversation

joestringer
Copy link
Member

@joestringer joestringer commented May 21, 2020

DNS servers may request a list of root nameservers by forming an NS
request for .. We have received reports that when applying a
visibility policy with the DNS matchPattern *, DNS requests of this
kind were being dropped in the proxy.

Fix this by extending the visibility match * to explicitly match on
either [validdnscharacters]., or .. If the matchPattern is more
complicated than simply *, do not match on ..

@joestringer joestringer added release-note/minor This PR changes functionality that users may find relevant to operating Cilium. needs-backport/1.6 labels May 21, 2020
@joestringer joestringer requested review from raybejjani and a team May 21, 2020 02:09
@maintainer-s-little-helper maintainer-s-little-helper bot added this to In progress in 1.8.0 May 21, 2020
@maintainer-s-little-helper maintainer-s-little-helper bot added this to Needs backport from master in 1.7.5 May 21, 2020
@maintainer-s-little-helper maintainer-s-little-helper bot added this to Needs backport from master in 1.6.9 May 21, 2020
@joestringer
Copy link
Member Author

test-me-please

@michi-covalent
Copy link
Contributor

lgtm, but i'm not sure if this was simply an oversight or there was some rationale for not matching this. i guess @raybejjani would know.

@coveralls
Copy link

coveralls commented May 21, 2020

Coverage Status

Coverage increased (+0.002%) to 37.064% when pulling ea739d8 on joestringer:submit/dns-match-root-ns into 59d1412 on cilium:master.

pkg/fqdn/matchpattern/matchpattern.go Outdated Show resolved Hide resolved
DNS servers may request a list of root nameservers by forming an NS
request for ".". We have received reports that when applying a
visibility policy with the DNS matchPattern "*", DNS requests of this
kind were being dropped in the proxy.

Fix this by extending the visibility match "*" to explicitly match on
either "[validdnscharacters].", or ".". If the matchPattern is more
complicated than simply "*", do not match on ".".

Signed-off-by: Joe Stringer <joe@cilium.io>
@joestringer
Copy link
Member Author

test-me-please

Copy link
Member

@nebril nebril left a comment

Choose a reason for hiding this comment

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

based on docs it seems like this is intended behaviour: * alone matches all names, and inserts all cached DNS IPs into this rule. via https://docs.cilium.io/en/v1.7/policy/language/#dns-based

@joestringer
Copy link
Member Author

Hit #11512 , #11645. CI otherwise green, and plenty of reviews.

Good to merge.

@joestringer joestringer merged commit 054dd16 into cilium:master May 21, 2020
1.8.0 automation moved this from In progress to Merged May 21, 2020
@joestringer joestringer deleted the submit/dns-match-root-ns branch May 21, 2020 20:06
@maintainer-s-little-helper maintainer-s-little-helper bot moved this from Needs backport from master to Backport pending to v1.7 in 1.7.5 Jun 3, 2020
@maintainer-s-little-helper maintainer-s-little-helper bot moved this from Backport pending to v1.7 to Backport done to v1.7 in 1.7.5 Jun 3, 2020
@maintainer-s-little-helper maintainer-s-little-helper bot moved this from Needs backport from master to Backport pending to v1.6 in 1.6.9 Jun 4, 2020
@maintainer-s-little-helper maintainer-s-little-helper bot moved this from Needs backport from master to Backport pending to v1.6 in 1.6.9 Jun 4, 2020
@maintainer-s-little-helper maintainer-s-little-helper bot moved this from Backport pending to v1.6 to Backport done to v1.6 in 1.6.9 Jun 4, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
release-note/minor This PR changes functionality that users may find relevant to operating Cilium.
Projects
No open projects
1.6.9
Backport done to v1.6
1.7.5
Backport done to v1.7
1.8.0
  
Merged
Development

Successfully merging this pull request may close these issues.

None yet

8 participants