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

root.hints permissions #8133

Closed
wants to merge 2 commits into from
Closed

root.hints permissions #8133

wants to merge 2 commits into from

Conversation

brotherla
Copy link
Contributor

sometimes, during interface reset, unbound config is regenerated/reloaded and I'm getting errors in unbound log like this:

 error: could not read root hints /root.hints: Permission denied

it looks like there could be 2 reasons for that:

  1. race condition: /var/unbound/unbound.conf file is regenerated , which triggers unbound process to reread it and thus reload root.hints, but at the same time new root.hints is created
  2. actually wrong permissions, default ones are 0600, so if there is some mismatch between file owner and unbound user - unbound cannot read it

so, by moving root.hints generation before unbound.conf generation and setting permissions to 0644 we can eliminate both reasons

usually rename preserves source file permissions, this is why permissions are set before rename, but to be 101% sure - chmod is used again after rename

@fichtner
Copy link
Member

fichtner commented Dec 14, 2024

I don’t mind adding chmod but moving the spot is probably not a good idea when we already talk race conditions.

@fichtner fichtner self-assigned this Dec 14, 2024
@brotherla
Copy link
Contributor Author

I don’t mind adding chmod but moving the spot is probably not a good idea when we already talk race conditions.

@fichtner why? by moving root.hints file generation before we make sure that during main conf file creation hints are already there and nobody is touching them

@fichtner
Copy link
Member

The history of this repeated tweaking is reason enough to be careful if you want to read it don’t take my word for it.

@brotherla
Copy link
Contributor Author

The history of this repeated tweaking is reason enough to be careful if you want to read it don’t take my word for it.

@fichtner I'm not sure that I understand you, what should I read? why moving the spot is not good idea?

@fichtner
Copy link
Member

I can neither see how the permission ends up 0600 or why we need to move the spot. The whole root.hints dilemma was due to a bug in Unbound startup that has been fixed. And the copy+rename is intended to eliminate potential for race conditions... it either opens the new inode or the old one. It should not fail.

@brotherla
Copy link
Contributor Author

I can neither see how the permission ends up 0600 or why we need to move the spot. The whole root.hints dilemma was due to a bug in Unbound startup that has been fixed. And the copy+rename is intended to eliminate potential for race conditions... it either opens the new inode or the old one. It should not fail.

@fichtner apparently it was not fixed, I was getting error messages in log every 1-2 days (I have one interface configured to restart every hour, thus chances of race conditions are quite high), after that fix I'm not getting any for 1 month already

@fichtner
Copy link
Member

Just to be sure we copy /usr/local/opnsense/data/unbound/root.min.hints which should have 0644 permissions like everything else. The one thing it doesn't have perhaps is unbound as user if unbound reads it under that user.

Canonically, this happens here:

chown -R unbound:unbound /var/unbound

In practice, start.sh runs under a lock and should prevent spurious reloads. You are running the latest 24.7.11 or something else?

@fichtner
Copy link
Member

PS: I'm just a bit pessimistic that moving a spot and changing permissions doesn't do anything and/or shift the race condition to a different user.

@sascha-jopen
Copy link

Hi all,

at least I can confirm that reloading of unbound occasionally leads to the error message mentioned by @brotherla. I do have connection resets on a DSL line once a day and roughly at every third reconnect unbound fails to reload. However, I haven't seen such behaviour when doing manual Unbound restarts. I think the problems started for me with a release of OPNsense in September or October, but I'm not sure.

@brotherla
Copy link
Contributor Author

@fichtner I'm running latest version OPNsense 24.7.11_2-amd64

source file has 0644 permissions, but when I was debugging - I saw 0600 (or 0640) permissions on target file, this is why I included chmod call

moving the spot - I cannot be 100% sure that it fixes the problem for every configuration, but I least it looks logical: first step is to generate inner file (hints), then generate outer file (main config) which references inner one

@fichtner
Copy link
Member

moving the spot - I cannot be 100% sure that it fixes the problem for every configuration, but I least it looks logical: first step is to generate inner file (hints), then generate outer file (main config) which references inner one

This definitely doesn't matter as Unbound is only ever started once all files are in place and the root hints are always there thereafter due to copy/overwrite approach.

If you can reproduce then try this between copy and rename calls:

chown($root_hints_tmp, 'unbound');
chgrp($root_hints_tmp, 'unbound');

@brotherla
Copy link
Contributor Author

@fichtner I did some testing with latest version, and it looks like the main problem is actually because of permissions, copy always creates 0600 (at least for me), so just by setting correct ones before rename solves the problem (no need to move spot), I've also tried chown/chgrp calls, but they actually not needed, though not harmful anyway

@fichtner fichtner closed this in 14bdcc9 Jan 17, 2025
@fichtner
Copy link
Member

@brotherla ok the 0644 certainly makes the thing work and it's not a secret file by any means. It could still be due to the unbound:unbound ownership missing but this is as good as it gets 14bdcc9 -- thank you for your report and help ❤️

fichtner added a commit that referenced this pull request Jan 17, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

3 participants