-
Notifications
You must be signed in to change notification settings - Fork 16
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
Psql bump #39
Psql bump #39
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.
Generally, adding support for PSQL 15/ 16 makes complete sense.
The conditional omission of stats_temp_directory
from the templates matches the breaking removal of this property from PSQL 15 - https://www.postgresql.org/docs/15/release-15.html
I am confused on the contract of the new privileges
item. Part of this confusion comes from the complexity of the existing logic in
ansible-role-postgresql/tasks/databases.yml
Lines 28 to 112 in e16285b
# Setting privileges is complicated: | |
# - https://stackoverflow.com/a/39029296 | |
# From https://www.postgresql.org/docs/9.6/static/sql-grant.html: | |
# | |
# "The key word PUBLIC indicates that the privileges are to be granted to | |
# all roles, including those that might be created later. PUBLIC can be | |
# thought of as an implicitly defined group that always includes all roles. | |
# Any particular role will have the sum of privileges granted directly to | |
# it, privileges granted to any role it is presently a member of, and | |
# privileges granted to PUBLIC." | |
# | |
# "There is no need to grant privileges to the owner of an object (usually | |
# the user that created it), as the owner has all privileges by default. | |
# (The owner could, however, choose to revoke some of their own privileges | |
# for safety.)" | |
# | |
# "PostgreSQL grants default privileges on some types of objects to PUBLIC. | |
# No privileges are granted to PUBLIC by default on tables, columns, schemas | |
# or tablespaces. For other types, the default privileges granted to PUBLIC | |
# are as follows: CONNECT and CREATE TEMP TABLE for databases; EXECUTE | |
# privilege for functions; and USAGE privilege for languages." | |
- name: postgres | revoke default permissions | |
postgresql_privs: | |
database: "{{ item.name }}" | |
privs: ALL | |
roles: PUBLIC | |
state: absent | |
type: database | |
when: "item.restrict | default(False)" | |
with_items: | |
- "{{ postgresql_databases }}" | |
changed_when: false | |
# Revoke the default permissions on the public schema | |
- name: postgres | revoke default schema permissions | |
postgresql_privs: | |
database: "{{ item.name }}" | |
obj: public | |
privs: ALL | |
roles: PUBLIC | |
state: absent | |
type: schema | |
when: "item.restrict | default(False)" | |
with_items: | |
- "{{ postgresql_databases }}" | |
changed_when: false | |
# The default public schema is owned by postgres, and since the PUBLIC | |
# privileges are revoked we must grant them back to the owner | |
- name: postgres | grant database owner public schema privileges | |
postgresql_privs: | |
database: "{{ item.name }}" | |
obj: public | |
privs: ALL | |
roles: "{{ item.owner }}" | |
state: present | |
type: schema | |
when: item.owner is defined | |
with_items: | |
- "{{ postgresql_databases }}" | |
- name: postgres | grant connect privileges | |
postgresql_privs: | |
database: "{{ item.1 }}" | |
privs: CONNECT | |
roles: "{{ item.0.user }}" | |
state: present | |
type: database | |
with_subelements: | |
- "{{ postgresql_users }}" | |
- databases | |
- name: postgres | grant usage privileges on default public schema | |
postgresql_privs: | |
database: "{{ item.1 }}" | |
objs: public | |
privs: USAGE | |
roles: "{{ item.0.user }}" | |
state: present | |
type: schema | |
with_subelements: | |
- "{{ postgresql_users }}" | |
- databases |
public
schema changes in PSQL 15 - see https://www.postgresql.org/docs/15/release-15.html. A few questions:
- did Molecule tests fail without this change on PSQL 15+? if so, are these genuine failures?
- , it would be useful to work through scenarios of how
postgresql_users
/postgresql_databases
maps into PSQL privileges - do the new
public
schema restrictions in PSQL 15+ overlap with the contract ofrestrict: True
? In that case, should we redefine the scope of this variable ?
The changes are related to the public schema.
Basically Since the
and not The problem occurs when creating the public db. |
Setting |
I think my concern is about the validity of the scenario tested by Molecule. In all the use cases I am aware of, tables should be created by the database owner e.g .
Exactly. So the way I see it, the second part of the
|
Last set of commits:
|
Discussed with @jburel earlier today, the new proposed behavior makes sense to me and is inline with the breaking privileges changes made in PSQL 15+ but it would be useful to have at least another review from the OME team The latest changes update the default to |
Considering that it is a security breach fixed in newer version of psql, I think it will make more sense to restrict all databases and drop the |
@pwalczysko @khaledk2 What do you think? |
README.md
Outdated
@@ -8,13 +8,15 @@ Install upstream PostgreSQL server. | |||
|
|||
Optionally creates users and databases. | |||
If you wish to use your distribution's packages then do not use this role. | |||
This role revokes default `PUBLIC` privileges from database and `public` schema for all supported versions of PostgreSQL. | |||
This is to be inline with the breaking privileges made in PostgreSQL 15. |
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.
This is to be inline with the breaking privileges made in PostgreSQL 15. | |
This is to be inline with the breaking privileges made in PostgreSQL 15. |
breaking privileges ? Maybe breaking changes
?
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.
adjusted
What will this change do with an already deployed psql database if I run the playbook with the postgresql role including this change on such already deployed psql DB ? |
Test 1: on
|
I think the main point from the above is that the following line from the output strongly suggests that this change will break the install of OMERO via ansible playbooks even on a clean system where the postgres-14,15 and OMERO.server is not installed. I would think that the OMERO.server role is trying to execute psql cmds during install where the schema used is
|
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.
This is not working for me atm - it seems that it is crashing on install of omero step. It seems that the omero-server role is using the public schema.
I can see that the tests here are not trying to install OMERO server which would be using this role - it seems that the problem here is indeed #39 (comment) |
The error means that the user |
I do not see any permissions setting possibilities inside the playbook.
|
i.e. makes |
But see below: the psql -l output is saying that the owner is still postgres.
|
I made an adjustment to the previous command |
This failed with
@sbesson is here and suggesting a test where I have a postgres 14 installed and then I just bump to 15 (ignoring this PR). Doing just that to verify. |
I can install postgres 15 and server successfully. But, in case there was a pre-existing running server with postgres 14, and I do following steps (stop and remove the postgres 14 service, remove the
|
I can deploy with this role on |
Merging and tagging as 5.4.0 |
This PR ads support for psql 15+
version 15 and 16 have been added to the testing matrix
I had to introduce a new parameter topostgresql_users
The changes should be backward compatibleThe changes are NOT backward compatible
i cherry-picked the commit from #33 and fix warning introduced
i will fix the other warnings in a follow-up PR to simplify the review of the required changes to support psql 15+
cc @technics3 mlukasik-dev
see https://www.postgresql.org/docs/release/15.0/ for background