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

[v1.13] fqdn: Add Protocol to DNS Proxy Cache #31713

Conversation

nathanjsweet
Copy link
Member

DNS Proxy indexes domain selectors by port
only. In cases where protocols collide on port
the DNS proxy may have a more restrictive selector than it should because it does not merge port
protocols for L7 policies (only ports).

Refactor all users of the DNS Proxy are updated
to add protocol to any DNS Proxy entries, and all
tests are updated to test for port-protocol
merge errors.

fqdn: Fixed bug that caused DNS Proxy to be overly restrictive on allowed DNS selectors.

[ upstream commit 1941679 ]

DNS Proxy needs to account for protocol when indexing
L7 DNS rules that it needs to adhere to, otherwise
L7 rules with differing port-protocols can override
each other (nondeterministically) and create overly
restrictive, and incorrect DNS rules. The problem with
accounting for protocol is that Endpoint restoration
logic uses DNS rules that index to port-only as JSON
saved to disk. Adding an additional protocol index to
a map structure changes the JSON structure and breaks
restoration logic between Cilium versions.

This change makes the map index backwards compatible,
since it changes the index from a uint16 to a uint32,
both of which marshal the same into a JSON structure.
The endpoint restoration logic will succeed between
versions, because the older version will be
automatically differentiated with a lack of a 1-bit
at bit position 24. Version 2 will save a 1 bit at the
24th bit going forward to differentiate when protocol
is indexed or not present.

Signed-off-by: Nate Sweet <nathanjsweet@pm.me>
[ upstream commit bc7fbf3 ]

DNS Proxy indexes domain selectors by port
only. In cases where protocols collide on port
the DNS proxy may have a more restrictive selector
than it should because it does not merge port
protocols for L7 policies (only ports).

All callers of the DNS Proxy are updated
to add protocol to any DNS Proxy entries, and all
tests are updated to test for port-protocol
merge errors.

Signed-off-by: Nate Sweet <nathanjsweet@pm.me>
[ upstream commit 6baab36 ]

DNSRulesV2 accounts for protocol and DNSRules does not.
DNSProxy needs to account for both, and endpoint needs
to be able to restore from a downgrade. DNSRulesV2 is used
by default now, but DNSRules is maintained in case of a
downgrade.

Signed-off-by: Nate Sweet <nathanjsweet@pm.me>
[ upstream commit abd7c6e ]

In cases where a port-protocol is not present
in an restored port protocol, look up
up the Version 1 version of the PortoProto
in case a Version 1 PortProto was restored.

Signed-off-by: Nate Sweet <nathanjsweet@pm.me>
[ upstream commit 5185252 ]

The Sort methods are updated to take an unused
testing.T structure to indicate to all callers
that they are only for testing purposes.

Signed-off-by: Nate Sweet <nathanjsweet@pm.me>
@nathanjsweet nathanjsweet added kind/backports This PR provides functionality previously merged into master. backport/1.13 This PR represents a backport for Cilium 1.13.x of a PR that was merged to main. labels Apr 1, 2024
@nathanjsweet nathanjsweet requested a review from a team as a code owner April 1, 2024 21:03
@nathanjsweet
Copy link
Member Author

/test-backport-1.13

@nathanjsweet
Copy link
Member Author

This is a backport of #31328

Copy link
Member

@pippolo84 pippolo84 left a comment

Choose a reason for hiding this comment

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

LGTM 🚀

@julianwiedmann
Copy link
Member

/test-1.22-4.19

@julianwiedmann
Copy link
Member

/test-1.26-net-next

@julianwiedmann
Copy link
Member

/test-1.26-net-next

@maintainer-s-little-helper maintainer-s-little-helper bot added the ready-to-merge This PR has passed all tests and received consensus from code owners to merge. label Apr 3, 2024
@julianwiedmann julianwiedmann merged commit 300b42c into v1.13 Apr 3, 2024
150 checks passed
@julianwiedmann julianwiedmann deleted the pr/nathanjsweet/backport-to-1-13-fix-dns-rules-policy-lockdown-bug branch April 3, 2024 07:26
nathanjsweet added a commit that referenced this pull request Apr 24, 2024
#31713 introduced
this break,
from a bad method signature hidden behind a build flag.

Fixes: #32163

Signed-off-by: Nate Sweet <nathanjsweet@pm.me>
ti-mo pushed a commit that referenced this pull request Apr 25, 2024
#31713 introduced
this break,
from a bad method signature hidden behind a build flag.

Fixes: #32163

Signed-off-by: Nate Sweet <nathanjsweet@pm.me>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport/1.13 This PR represents a backport for Cilium 1.13.x of a PR that was merged to main. kind/backports This PR provides functionality previously merged into master. ready-to-merge This PR has passed all tests and received consensus from code owners to merge.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants