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

Fix DDNS keyfile creation, configuration and writeable zone location #59

Merged
merged 6 commits into from
Feb 15, 2023

Conversation

0xMattijs
Copy link
Contributor

Problem

This Ansible role writes zone files to a hard coded /etc/bind/zones directory, which causes problems on systems with mandatory access control such as Apparmor. The profile for Apparmor does not allow the BIND process to create the necessary journal files and update the zone files under /etc/bind/zones.

Solution

Parameterize the zone directory and default to /var/lib/bind/zones, for which write access is allowed by the Apparmor profile.

PR

This PR parameterises the location for storing the zone files. It also includes code to generate DDNS keys using tsig-keygen if a configured update_keyfile does not exist already. Since tsig-keygen generates a full key configuration section, the named.conf.options.j2 template has been adjusted accordingly.

@doobry-systemli
Copy link
Contributor

Dear @0xMattijs, thanks for your contribution!

After a first look I have two comments:

  1. I'd prefer to keep the default path for keys at /etc/bind/keys to not break backwards-compability. Since you make it configurable anyway, I think that should be fine.
  2. The tasks to optionally create a DDNS key result in changes in your local playbook. While I see the advantage of providing code to do so, I don't like this being executed per default. Most setups I know have their Ansible playbook in a Git repository and altering that while deploying is unexecpted behaviour in my eyes. Maybe instead it would be an option to run the tasks only if a variable is set to true (e.g. with ansible-playbook ... -e 'bind9_generate_ddns_key=true') and document that behaviour in README.md? What do you think about that approach?

0xMattijs and others added 2 commits February 1, 2023 13:51
…/zones` in order to preserve backwards compatibility

Make DDNS key generation optional using `bind9_generate_ddns_key` (default: false)
Make key location configurable using `bind9_local_keydir` (default: files/bind/zones)
@0xMattijs
Copy link
Contributor Author

0xMattijs commented Feb 1, 2023

Hi @doobry-systemli . Totally makes sense. I have processed your suggestions in the PR:

  • Make generating keys optional with bind9_generate_ddns_key
  • Adjusted the default zone DB location back to /etc/bind/zones
  • Adjusted the default key storage location to files/bind/zones within the playbook (the location where the role expects them when provided by the user)
  • Updated the README

Copy link
Contributor

@doobry-systemli doobry-systemli left a comment

Choose a reason for hiding this comment

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

Thanks a lot for adressing my comments @0xMattijs. Sorry for the delay with my response. The PR looks good to be merged now - just a minor comment.

README.md Outdated Show resolved Hide resolved
@doobry-systemli doobry-systemli merged commit 1e15664 into systemli:main Feb 15, 2023
@0x46616c6b 0x46616c6b added chore enhancement New feature or request bug Something isn't working and removed chore enhancement New feature or request labels May 9, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants