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

nixos/bind: Fix cacheNetworks option #335832

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Conversation

TobTobXX
Copy link

@TobTobXX TobTobXX commented Aug 19, 2024

Description of changes

services.bind.cacheNetworks should only apply to recursive queryies, as per the option documentation:

Note that this is for recursive queries – all networks are allowed to
query zones configured with the zones option by default [...].

This would correspond to the allow-query-cache option in named.conf, as per the BIND docs1:

Specifies which hosts (an IP address list) can access this server’s
cache and thus effectively controls recursion.

And not allow-query, which restricts all requests (including requests where the server has authority) 2:

Specifies which hosts (an IP address list) are allowed to send queries
to this resolver.
[...]
Note:
allow-query-cache is used to specify access to the cache.

Things done

  • Built on platform(s)
    • x86_64-linux
    • aarch64-linux
    • x86_64-darwin
    • aarch64-darwin
  • For non-Linux: Is sandboxing enabled in nix.conf? (See Nix manual)
    • sandbox = relaxed
    • sandbox = true
  • Tested, as applicable:
  • Tested compilation of all packages that depend on this change using nix-shell -p nixpkgs-review --run "nixpkgs-review rev HEAD". Note: all changes have to be committed, also see nixpkgs-review usage
  • Tested basic functionality of all binary files (usually in ./result/bin/)
  • 24.11 Release Notes (or backporting 23.11 and 24.05 Release notes)
    • (Package updates) Added a release notes entry if the change is major or breaking
    • (Module updates) Added a release notes entry if the change is significant
    • (Module addition) Added a release notes entry if adding a new NixOS module
  • Fits CONTRIBUTING.md.

Add a 👍 reaction to pull requests you find important.

@TobTobXX TobTobXX requested a review from peti as a code owner August 19, 2024 14:40
@NixOSInfra NixOSInfra added the 12. first-time contribution This PR is the author's first one; please be gentle! label Aug 19, 2024
@github-actions github-actions bot added 6.topic: nixos Issues or PRs affecting NixOS modules, or package usability issues specific to NixOS 8.has: module (update) This PR changes an existing module in `nixos/` labels Aug 19, 2024
@TobTobXX
Copy link
Author

TobTobXX commented Aug 19, 2024

  • There's nixos/test/bind.nix and I don't know how to run it.
  • Is this breaking in some way? It should only bring it close to what the docs said it should do.

@wegank wegank added the 2.status: merge conflict This PR has merge conflicts with the target branch label Sep 27, 2024
@Zocker1999NET
Copy link
Contributor

I agree with you that allow-query-cache is the better option considering the description of cacheNetworks. Through, it might be that some users are relying on allow-query to be defined by cacheNetworks, so I would add a release note nonetheless.

Also, I catched that the services.bind.zones.<name>.allowQuery option’s description explains that allow-query is set by cacheNetworks, so I think this description should be adapted as well (IMO that additional note probably can be removed).

  • There's nixos/test/bind.nix and I don't know how to run it.

Just in case you haven’t figured it out yet: with your branch checked out, from the nixpkgs root, run: nix-build -A nixosTests.bind or nix build .#nixosTests.bind (with flakes enabled).

On the current commit 8f16ef25cb2ee37cb1018e6cddaab82d93868858 I ran following tests, which were all successful:

@TobTobXX
Copy link
Author

TobTobXX commented Oct 6, 2024

I'll implement your recommendations soon. It took me quite some time just to rebase the PR and I ran out of time for this week. Thank you for your great review though.

@ofborg ofborg bot added 10.rebuild-darwin: 0 This PR does not cause any packages to rebuild on Darwin 10.rebuild-linux: 1-10 and removed 2.status: merge conflict This PR has merge conflicts with the target branch labels Oct 6, 2024
@TobTobXX
Copy link
Author

TobTobXX commented Oct 7, 2024

Ok nevermind, I could implement the changes just now.

  • Add changelog entry
  • Run tests

The PR is now ready for review again.

Copy link
Contributor

@Zocker1999NET Zocker1999NET left a comment

Choose a reason for hiding this comment

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

LGTM with changing the doc & adding that short release note

@TobTobXX
Copy link
Author

TobTobXX commented Oct 28, 2024

Rebased again. @peti is there something blocking this?

Copy link
Member

@peti peti left a comment

Choose a reason for hiding this comment

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

👍, looks good to me.

@wegank wegank added the 12.approvals: 2 This PR was reviewed and approved by two reputable people label Oct 29, 2024
@wegank wegank added the 2.status: merge conflict This PR has merge conflicts with the target branch label Nov 9, 2024
@TobTobXX
Copy link
Author

I just rebased this for the I-dont-know-how-many-th time.

  • How should I write the changelog line so that there are no merge conflicts? Is there a styleguide I missed?
  • Whom do I need to ping to get this finally merged?

@wegank wegank removed the 12.approvals: 2 This PR was reviewed and approved by two reputable people label Nov 19, 2024
services.bind.cacheNetworks should only apply to recursive queryies, as
per the option documentation:
> Note that this is for recursive queries – all networks are allowed to
> query zones configured with the zones option by default [...].

This would correspond to the `allow-query-cache` option in named.conf,
as per the BIND docs[1]:
> Specifies which hosts (an IP address list) can access this server’s
> cache and thus effectively controls recursion.

And not `allow-query`, which restricts all requests (including requests
where the server has authority) [2]:
> Specifies which hosts (an IP address list) are allowed to send queries
> to this resolver.
> [...]
> Note:
> `allow-query-cache` is used to specify access to the cache.

[1]: https://bind9.readthedocs.io/en/v9.20.0/reference.html#namedconf-statement-allow-query-cache
[2]: https://bind9.readthedocs.io/en/v9.20.0/reference.html#namedconf-statement-allow-query
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
2.status: merge conflict This PR has merge conflicts with the target branch 6.topic: nixos Issues or PRs affecting NixOS modules, or package usability issues specific to NixOS 8.has: changelog 8.has: documentation 8.has: module (update) This PR changes an existing module in `nixos/` 10.rebuild-darwin: 0 This PR does not cause any packages to rebuild on Darwin 10.rebuild-linux: 1-10 12. first-time contribution This PR is the author's first one; please be gentle!
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants