-
-
Notifications
You must be signed in to change notification settings - Fork 160
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
feat: limit /etc to readonly #1451
base: develop
Are you sure you want to change the base?
feat: limit /etc to readonly #1451
Conversation
51b7090
to
e4f3d5e
Compare
7622ed8
to
9460933
Compare
@staaldraad I think I need to rebase this and get some tests (including adding to our osquery test) |
https://www.freedesktop.org/software/systemd/man/latest/systemd.exec.html#ReadWritePaths= Prevent postgres, or child-process of, from writing to /etc
9460933
to
0012206
Compare
rebased - and will look at the osquery test |
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.
Just recommending that we can expand our existing permissions testing to make sure this doesn't regress
Those tests live https://github.com/supabase/postgres/blob/develop/ansible/files/permission_check.py
Could (for instance) throw in another query related to this setting, or expand existing query
Not finding a great way to retrieve this with osquery. Can use osquery to get the Otherwise, the What could work is getting the
|
I think the way you described it is the best way to do it
Basically this seems like what you described already, which seems to be the best way to accomplish this kind of test. The test could be unstable or fail if you cannot pin down the correct process to check, possibly. Thank you for doing this! If you feel like you have a better way that works than what I describe, of course please pursue that. |
....if there is a better/simpler way that does not use osquery that should be fine too, and you could still throw it into the this python file |
@@ -151,6 +153,33 @@ def check_nixbld_users(): | |||
|
|||
print("All nixbld users are in the 'nixbld' group.") | |||
|
|||
def check_postgresql_mount(): |
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.
Nice thank you!
@staaldraad do you think this is likely ready at this point? |
@samrose yes. Will co-ordinate a phased rollout, but this actual change is ready |
ansible/vars.yml
Outdated
@@ -8,8 +8,8 @@ postgres_major: | |||
|
|||
# Full version strings for each major version | |||
postgres_release: | |||
postgresorioledb-17: "17.0.1.053-orioledb" | |||
postgres15: "15.8.1.060" | |||
postgresorioledb-17: "17.0.1.054-orioledb" |
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.
Ok, before we merge, let's add a suffix to these version numbers here like
postgresorioledb-17: "17.0.1.054-orioledb-etc-1"
Then sometime today we can test this in local infra, prior to merge.
Ok, before we merge, let's add a suffix to the version numbers as described here https://github.com/supabase/postgres/pull/1451/files#r2013958022 We're trying to make sure we get all significant changes tested in local infra before we roll out, to avoid failure incidents. |
@staaldraad I added that version suffix here 1083e1f If you don't yet have a local infra set up to test pause/restore, upgrade, and other scenarios yourself, I can do this sometime today. |
beat me too it, no wonder I couldn't push :) thanks! yeah I can run through the testing locally again |
What kind of change does this PR introduce?
Feature
What is the current behavior?
Please link any relevant issues here.
What is the new behavior?
https://www.freedesktop.org/software/systemd/man/latest/systemd.exec.html#ReadWritePaths=
Prevent postgres, or child-process of, from writing to /etc