-
Notifications
You must be signed in to change notification settings - Fork 122
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: reduce mode of SQLite storage file to 0o600 #1057
fix: reduce mode of SQLite storage file to 0o600 #1057
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good, thanks!
I wouldn't block this, but I would say that if we really want to care about the security of this file, we might as well take the step of creating it secure in the first place to avoid ever having a file handle that an attacker could exploit. |
Moving back to draft until I verify the new approach with a real (non-unit, manual) test. |
Manually verified the simple cases on micro-k8s (no existing state file, existing state file). |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good, and thanks for the tests. Per discussion, we'll fail hard in both cases. As Tony said on Mattermost:
It does seem like if we are going to add this (minimal) layer of security then we ought to ensure that it either happens or fails, so that people don't assume that it's happening when it's not.
sqlite3 creates new database files with 0o600 permissions. Although Charms should generally not store private data in the backend storage, this does happen sometimes, and it would be nicer to ensure that only the user (that is, root) can access the underlying storage file.
This chmod's the file (to
-rw-------
) after every open even if it already exists. It seems simpler to do this and get the additional access fix rather than only do it on initial creation.Fixes #1051.