perf: avoid unconditional chmod -R in enforce_misp_data_permissions#378
perf: avoid unconditional chmod -R in enforce_misp_data_permissions#378ostefano merged 4 commits intoMISP:masterfrom
chmod -R in enforce_misp_data_permissions#378Conversation
…are actually incorrect Replace the three unconditional `chmod -R` calls with targeted `find` expressions that only touch files whose permissions are actually incorrect.
|
Wow TIL. Will test and merge it at the next opportunity. LGTM, thanks! |
|
I have a concern now that I think of it. What if a dir is set to be 755, but then, something or someone chmod it to 777? |
`-not -perm /0550` to ` ! -perm 0550` (exactly Match) `-not -perm /0770` to `! -perm 0770` (exactly Match) `-type f / -type d` ( find filter for file type) - delete `chmod -R u+w,g+w` lines – because its redundant to ` ! -perm 0550/0770`
|
You're right that That would be even stricter/better than in the original. :) |
|
This is becoming difficult to review. Can you retrofit our discussion in the main description of the issue so we can ask other folks to review? Thanks! |
|
Okay, I'll do it, or rather, the AI will ;) I hope the new description is what you had in mind? |
|
Thanks, I will give it a proper look later today 👍 @UFOSmuggler could you do a pass here as well (rather critical area) |
|
Ok so to me it looks like the real issue is that after some This means every container start, we change the file perms in some paths twice. presumably this is an accident. We make this possible mistake in the following locations:
Removing ONE of these recursive file mode changes is a good move. However, which to remove? Do we want 0550 or 0770? It looks like 0770 is needed at least in SOME locations. I don't yet know why, but if I spin up a fresh container using this PR, it starts healthily. I then start pulling the circl.lu feed and it works. I then restart the container, and it spams a lot of shit into the logs: And so on... I don't yet know why this is... I assume maybe cake console wants to write shit as www-data in Otherwise the diff seems to show stylistic differences in the syntax of the commands which are as far as i can tell functionally equivalent, with some possible portability differences. ESOTERIC AND DOESN'T REALLY MATTER: I wondered if this made a difference, which it doesn't really appear to, except perhaps for posix portability in at least one case. Using the Old:
I assume the thinking was "find files without the special bit set", but this is what already happens with The only other real difference seems to be the order of the file type and permission tests, which don't seem to matter very much. Each version of the command with I created a million empty files over a thousand directories and tested the two (on a very fast pcie5 nvme disk): |
|
I don't think I can look further into this until tomorrow. |
|
770 will indeed be needed for quite a few files in there. The cache files for one, are frequently updated by the framework. Those directories also include temporary files used for exports, sadly it's a pretty bad mish-mash of script files and temporary user data. One thing that could work is setting 770 on the entire directory and 550 on anything .py - though 770 is pretty low risk too. |
|
getting rid of the non-find chmod is the right move, and maybe just do one find chmod to enforce 770 on both dirs and files, where appropriate. no -type parameter. 770 is what we already had, is not a step backwards. we can do further tightening work later to address what has been illuminated by this PR. |
|
*Also, add mental note to correctly split data from scripts in files for easier maintenance in 3.x |
great idea, make an issue or jira ticket or whatever you guys use. harder to misplace than mental notes. at least in my considerable career of misplacing mental notes. |
|
Thank you, @UFOSmuggler, for the excellent and interesting analysis, and thank you, @iglocska , for your additions. The comparison between I strongly support the suggestion regarding the separation of data and scripts in 3.x. However, I will now take it upon myself to favor the following solution and adapt it accordingly.
|
solution from discussion: - app/tmp + app/files → everything 0770 without -type, a single find per directory – exactly what ufoSmuggler suggested - app/Config → remains differentiated: Files 0550, Dirs 0770 – Config files should not be writable - chmod -R over all files in the end, completely gone – that was the original perfomance problem
|
this looks good to me now. thanks for your work @LSI-ZuagrastaWastl 🫡 |
Problem
On deployments with large
app/filesvolumes (tested with ~90GB),enforce_misp_data_permissions()inentrypoint_nginx.shcaused significantstartup delays on every
docker compose up– even when permissions were already correct.Root causes
chmod -R u+w,g+w <dir>ran unconditionally over every file on every start,regardless of whether permissions were already correct.
-not -perm 770without a leading/or-is an exact octal match infind,not a bitmask check. Files with broader permissions like
0777or0755wereunnecessarily touched every run.
Fix
Replace all unconditional
chmod -Rcalls with targetedfindexpressions usingexact
! -perm XXXXmatching:find -type f ! -perm 0550 -exec chmod 0550 {} +find -type d ! -perm 0770 -exec chmod 0770 {} +! -perm 0550(without/or-) is an exact match in find – it catches both directions:0444) → corrected ✅0777) → corrected ✅0550→ skipped entirely ✅The
chmod -R u+w,g+wlines are removed entirely as they are now fully coveredby the exact permission matching above. Notably the original
chmod -R u+w,g+wwould never have corrected
0777back to0550either – so the new fix isstrictly more correct in that regard as well.
Result
~0 chmod syscalls → startup reduced from several minutes to seconds
0777): now explicitly corrected, which the original did not doOpen discussion
@ostefano raised a valid objection:
What if a dir is set to be 755, but then, something or someone chmod it to 777?
I believe the original spirit of this "enforcing phase" was exactly to fix these issues because MISP was not to be trusted 100% on this when operating on shared volumes (docker limitations).
But this was not taken with the original solution
chmod -R u+w,g+w.But the Fix
find -type f ! -perm 0550 -exec chmod 0550 {} +find -type d ! -perm 0770 -exec chmod 0770 {} +should also fixes this case.