Don't change owners in /var/mail recursively#2256
Don't change owners in /var/mail recursively#2256MohammedNoureldin wants to merge 8 commits intodocker-mailserver:masterfrom
Conversation
Changing the whole owners of folders inside */var/mail* ruins the whole LDAP users permissions on their own mail folders. Thus, only */var/mail* owner should be fixed if necessary (not recursively).
There was a problem hiding this comment.
Glanced over history for this logic, and I do not see any strong justification for keeping the original.
If tests pass then it's fine by me. Should any issues actually surface, then we have new tests to add :)
While unrelated to this PR, I want to make mention of a permissions addition in the past for reference:
- March 2017 PR that added various
chown -Rfor services and Postfix config to resolve issues. - Issue comment that advised
chown -Rfor/var/mail-state.
| if [[ $mail_owner_user -ne 5000 || $mail_owner_group -ne 5000 ]]; then | ||
| _notify 'inf' 'Fixing /var/mail permissions' | ||
| chown -R 5000:5000 /var/mail || _shutdown 'Failed to fix /var/mail permissions' | ||
| chown 5000:5000 /var/mail || _shutdown 'Failed to fix /var/mail permissions' |
There was a problem hiding this comment.
When originally introduced this chown -R line was not explained beyond the associated line "Fixing permisions".
That line has been present since the very first commit of the project (March 2015).
In Dec 2016 an issue regarding permissions on /var/mail was raised, the related code is brought up, the original project author responds and doesn't explain the need for chown -R, they don't appear to be aware of it's requirement.
Related issue, but focused on the GID instead of UID for file access on host storage for multiple users under a different GID: #1611
Related issue, user migrates existing IMAP mailboxes by copying over files on the host from a source not previously handled by docker-mailserver. There issue with permissions was that they could not move/delete directories via their MUA (Thunderbird): #2208
| # fix permissions, but skip this if 3 levels deep the user id is already set | ||
| if find /var/mail -maxdepth 3 -a \( \! -user 5000 -o \! -group 5000 \) | read -r | ||
| then |
There was a problem hiding this comment.
This appears to have been added to reduce startup overhead of redundantly applying the permissions to contents of multiple GB in size.
Related, this code snippet has been copied into check-for-changes.sh, which will also apply chown -R 5000:5000, as identified here.
You will want to apply your changes to this too:
docker-mailserver/target/scripts/check-for-changes.sh
Lines 217 to 220 in 34ba3c2
|
9 tests fail. They must be compatible with this PR if you want to get it merged. Investigate why it's an issue for them and resolve it if possible. Test failures all with trouble accessing `mail_hostname.bats`: 52 checking configuration: hostname/domainname override: check headers of received mail`mail_hostname.bats`: 58 checking configuration: non-subdomain: check headers of received mail`mail_lmtp_ip.bats`: 68 checking postfix-lmtp: delivers mail to existing account`mail_privacy.bats`: 88 checking postfix: remove privacy details of the sender`mail_spam_junk_folder.bats`: 105 checking amavis: spam message is delivered and moved to the Junk folder (MOVE_SPAM_TO_JUNK=1)`mail_spam_junk_folder.bats`: 106 checking amavis: spam message is delivered to INBOX (MOVE_SPAM_TO_JUNK=0)`mail_special_use_folders.bats`: 108 checking normal delivery`mail_with_mdbox.bats`: 159 checking dovecot mailbox format: mdbox file created`mail_with_sdbox.bats`: 183 checking dovecot mailbox format: sdbox file created |
|
Thanks for providing a PR. Please now have a look at the tests failing to identify the issues. You could for example only turn off recursive changes when LDAP is enabled. |
|
I don't understand this PR. So there is an issue with permissions/ownership when LDAP is involved. Fixing an LDAP issue, while introducing possible issues for existing non LDAP setups doesn't seem a proper way to do it. With this "fix" applied, what happens when files in subdirectorys of |
I think the assumption was that it was not clear what exactly required Based on what information I could lookup via Are you saying that dovecot requires
I don't know if that's an appropriate "fix" if the tests would still fail with LDAP settings. I'm reluctant to approve the PR if it's prone to subtle failures elsewhere that the author hasn't run into themselves. I haven't looked at the failing tests, so perhaps the risk isn't there for LDAP 🤷♂️ |
|
@polarathene you're right, I wouldn't approve this either as long as tests fail. |
If you migrate to a new server/storage etc, or forget to backup permissions as well, the permissions in the data volume may be incorrect. I guess, this fix ensures that all permissions are as expected and the files are accessible by the various services.
I think so, but cannot confirm this 100%. This was just an assumption based on the current script. I suppose, no one would introduce a fix for permissions, when there aren't any issues that could arise. |
|
Hi guys, sure I will take a look on the tests that fail in the evening. @casperklein overwriting the current permissions is definetely not a fix, this is in best cases a workaround to get things working when the user uses somehow wrong permissions (like copying the folders from another system without their permissions). However, this destroys the whole permissions for the other users who know that they really need custom permission configurations. IMO, the least painful and risk form of this workaround is to keep it but do not use it if LDAP is used, though this is not nice at all, it will work at least for all (hopefully). But for me it seems like it can be removed safely, as it is not needed to recusively change all permissions, only changing /var/mail should be sufficient. Or what do you think? From what I saw quickly, Dovecot does not need necessarily uid 5000 to work, I myself use different uid -> different owners of folders and it works perfectly, of course, till I restart my container and all owners are overwritten to 5000 :(. Sorry I cannot reply to all comments now as I am doing a short break, but I will take care about them as said in the evening. |
|
Hello again @polarathene @georglauterbach @casperklein, I took a look on the tests, I don't fully understand why it is not able to find a specific folder under /var/mail, probably it is not able now to write these folders, but I am not sure why. So here I need some help from you please, what would you suggest either to fix the tests (I am not sure if fixing the tests is a good idea here), or do you suggest anything else to change in the code to make the tests no fail? What would you suggest? Will only checking if LDAP is activated and execute the code otherwise sufficient? I still have the opinion that this recursive owner changing is not required, and is rather not good, but I don't want to trigger a big refactoring here. Any small fix that gets things to work properly for both LDAP and non-LDAP is fine. Any suggestions what I should do further? |
I was more referring to not hacking around the tests (eg: a feature flag that applies this PR only to LDAP users, when the failing tests are not configured for LDAP, so the tests pass), or modifying the tests to pass in a manner that hides the issue as well.
I already pointed out that case as one of a few related to the issue this PR is addressing in this reveiw comment. Although it's not clear why that's happening.
See the same link I just provided. This "fix" was introduced in the very first commit of the project by the original author. They've not always made sound choices and didn't seem to explain it's purpose during a relevant discussion I linked, I can't say I'm confident in the choice based on the history I dug up. They likely went with it because "it works for me" as a fix at the time.
Without looking into the failing tests, I would like to know that they would pass for an LDAP setup if you applied your PR via a feature toggle (doesn't have to be LDAP specific). My concern is that if tests are failing, just because you don't notice any issues doesn't mean that there are none, or that it won't affect other LDAP users as a result. This would be less of an issue by using an opt-in ENV, but I am not particularly fond of that either without a proper understanding of why it's needed (not your requirement, but why we can't use it by default due to tests failing).
Permissions....? That's literally what this PR is changing. The good news is that they were all What might work is just recursively adding the GID 🤷♂️ But then that should work for you if your LDAP users belonged to the 5000 group within When you have identified the failure cause for tests and can detail it here, we can discuss further.
You may consider adding an ENV to ignore applying the Personally I would prefer you still look into the test failures for more information as to why they're failing, as if you add that ENV it should probably come with supporting tests anyway.
While I agree, we need to understand why that is not the case with the tests first. I was not able to dig up anything in project history that demonstrates what the permissions are being fixed for. We do thankfully have failing tests that should help enlighten us on that, but I do not have the time to do so myself. If you want to resolve this, you'll need to investigate and find this information.
Yes. If you do not want to pursue the above, you can choose to volume mount your fix over the related file within the container on your end instead of upstreaming it here. I assume The other maintainers may be fine with the opt-in ENV to disable the |
When I look in |
So the tests are failing because we're providing data that's not using If so, then we can safely have that data
I don't mind keeping it, when it's clear what the permission fix is actually required for. It should then be more explicitly mentioned that we're fixing permissions for that purpose. But currently, what I'm hearing is that at runtime without LDAP, Dovecot works with
If it can be identified that it's actually an issue for non-LDAP environments. No point adopting a new workaround ENV if the problem wouldn't ever occur regardless when |
|
@polarathene @casperklein I am working quickly on restoring the old condition and adding a new condition to check if LDAP is active. Nevertheless, I am still a bit not very comfortable with this, because I don't like having this chown -R at all, which may ruin my permissions somehow in the future, while we don't really know why it is needed. Do you suggest anything else as a hotfix for this? |
Distinguish between LDAP actived and disabled cases when changing the owners in */var/mail* to avoid overwriting the permissions and owners of mail folders for LDAP users.
I won't approve of that unless proven that it's required. Add an opt-in ENV instead. I still prefer you actually look into the test failures and the cause. Your quite possibly wasting effort on an alternative when your original PR here can be accepted provided you look into the test failures instead of hand-waving the failure away behind an ENV. If you add the opt-in ENV, I want to know that those test failures will pass with the ENV when the tests are configured for LDAP. Otherwise, you will need to document that the ENV is not supported officially and may risk unexpected breakage/failures.
Then investigate it as has been suggested. You've been provided insights in the discussion here already.
You can maintain the patch on your end, as was suggested earlier if you don't want to do what is required for upstreaming a fix. |
I will work on getting more time to investigate this. Nevertheless, I would appreciate if anyone can check the first test that fails and tell me what he thinks about it when you have time. Because it was not really clear for me why the file is not being written to the expected folder and why is this happening.
I would like it merged to the upstream, so nobody else faces this issue :) |
georglauterbach
added
meta/needs triage
|
This has stalled significantly. As part of #3289, I will close this (see my rationale here: #3289 (comment)). @MohammedNoureldin this does not mean these changes are not worthwhile; I just cannot maintainer PRs that are years old. When you have the time, please propose a working set of changes on the most recent version of |
|
This was intended to be investigated once the test suite was fully migrated to the new structure. LDAP did not get to that point for v12. This has not been an issue raised by many other LDAP users AFAIK, there has been little involvement since the issue / PR was opened. If someone were to be faced by this problem and wants to resolve it, my advice is to do the following:
Seeing as it doesn't affect all LDAP users, I assume it's config specific or affected by choice of LDAP provider. |
|
Hi, thanks for notifying me @georglauterbach, I will check it again for sure once I have more time again. @polarathene it is kind of configuration-specific, but not fully. If the administrator is OK to get all emails owned by the user 5000, then the issue will not appear. On the other hand, if every single LDAP user (with unique ID) should own only his own emails on the Linux system ( So it will not appear in the less-secure option I would say, but it should be definitely be checked somewhen. |
|
Specifically, I think this is caused by including When using LDAP, you can tell Dovecot to get the uid:gid from the database, which it will then switch to when accessing mail. Since the directories are all chowned to 5000:5000, this causes Dovecot to not actually be able to access the user's mail. You can manually correct the permissions, which makes Dovecot work correctly, but I was still having delivery problems. I was using |
The discussion above had attempted to only apply the We apply this A lot has happened with the project codebase since this PR was attempted, quite a bit of refactoring in our test suite and code related to this PR/issue. |
6 participants
Description
Changing the whole owners of folders inside /var/mail ruins the whole LDAP users permissions on their own mail folders. Thus, only /var/mail owner should be fixed if necessary (not recursively).
Fixes: #2238
Type of change
Checklist:
docs/)