Remove upstream-breaking REVOKE CONNECT * FROM public

[mzagrabe]

The default installation of Pg allows public to connect - given proper pg_hba entries. The REVOKE subltely breaks expected usage.

Summary

Provide a detailed description of all the changes present in this pull request.

Additional Context

Add any additional context about the problem here.

• Root cause and the steps to reproduce. (If applicable)
• Thought process behind the implementation.

Related Issues (if any)

Mention any related issues or pull requests.

Checklist

• 🟢 Spec tests.
• 🟢 Acceptance tests.
• Manually verified. (For example puppet apply)

[CLAassistant]

All committers have signed the CLA.

[ekohl]

I tried to understand why it was even there, but it's ancient code. 1175ea2 is the earliest version where it was introduced. After that it was only minor stylistic changes or moving it around. Given that's 12 years old I wonder if it was a workaround for something.

Do you have any idea?

[mzagrabe]

I tried to understand why it was even there, but it's ancient code. 1175ea2 is the earliest version where it was introduced. After that it was only minor stylistic changes or moving it around. Given that's 12 years old I wonder if it was a workaround for something.

Do you have any idea?

Yup. I looked at its origin as well. The commit doesn't lend itself to understanding why it was introduced. The comment in the code makes it seem like it is for "security" purposes. I suppose in theory it is more secure, but in practice I'm dubious of its security related benefits. Nevertheless, it deviates from upstream defaults and makes things more challenging when working with folks who don't know that this REVOKE is altering things.

I know this would be a backwards incompatible change, but I'd vote for merging the PR.

[mzagrabe]

Any idea what it will take to get this PR merged?

[ekohl]

You signed the CLA (IIRC you didn't when I last looked) and we can ignore the mend check. Not sure if I've ever seen that green.

Reading up on this (https://www.red-gate.com/simple-talk/databases/postgresql/postgresql-basics-roles-and-privileges/) tells me that in PostgreSQL < 15 any user could CREATE objects if they had CONNECT permissions. You can also (including >= 15) read schema information. As such I think this change has security implications.

Because of that I'd at the very least treat this as a breaking change. Even better if we can figure out why it breaks. My reading is that it should work and perhaps there's another solution that maintains the hardening

[mzagrabe]

Hello @ekohl

You signed the CLA (IIRC you didn't when I last looked) and we can ignore the mend check. Not sure if I've ever seen that green.

Good to know.

Reading up on this (https://www.red-gate.com/simple-talk/databases/postgresql/postgresql-basics-roles-and-privileges/) tells me that in PostgreSQL < 15 any user could CREATE objects if they had CONNECT permissions. You can also (including >= 15) read schema information. As such I think this change has security implications.

There is still the pg_hba that affects whether users can login and what they can login to.
Not all security would be lost with this change.

The article reference above does state:

"""
As a database administrator you could REVOKE the ability for the PUBLIC role to CONNECT and then grant it to each role individually, but the complexity of managing that is rarely worth the effort.
"""

Also, the default upstream is to allow connect to public.

Nevertheless, I agree that merging this commit would be a breaking change, which you state below.

Because of that I'd at the very least treat this as a breaking change. Even better if we can figure out why it breaks. My reading is that it should work and perhaps there's another solution that maintains the hardening

What do you mean by "figure out why it breaks"? Are you talking about the module before my commit - which revokes connect and breaks what upstream ships? or are you talking about post-merge of my commit, which would be a breaking change for the module? Either way, I thought we knew "why" it is 'breaking'.

Also, what do you mean by "it should work"? I thought it was obvious why it didn't work.

Thanks for filling in the gaps for me.

Cheers,

-m

[ekohl]

What do you mean by "figure out why it breaks"? Are you talking about the module before my commit - which revokes connect and breaks what upstream ships? or are you talking about post-merge of my commit, which would be a breaking change for the module? Either way, I thought we knew "why" it is 'breaking'.

I'd like to know how it breaks.

The REVOKE subltely breaks expected usage.

If you could provide any details or even better, a reproducer where something relies on the CONNECT permission to a database.

[mzagrabe]

What do you mean by "figure out why it breaks"? Are you talking about the module before my commit - which revokes connect and breaks what upstream ships? or are you talking about post-merge of my commit, which would be a breaking change for the module? Either way, I thought we knew "why" it is 'breaking'.

I'd like to know how it breaks.

Ah. Yes.

I am trying to create read-only admins on for my Pg server to be able to view any database and corresponding objects in any database:

postgresql::server::role { "foo":
    username => "foo",
    superuser   => false,
    createdb    => false,
    createrole  => false,
    replication => false,
    password_hash => postgresql_password(
                "foo",
                $password_to_use,
    ),
}

and I've got the following command being exec'ing via psql in puppet:

GRANT pg_read_all_data TO foo;

and I've got the correct pg_hba entry to allow local login. But I cannot connect to any of the databases on the system that were created with puppet (due to the REVOKE).

However, if I manually create a database, I can connect and perform read-only operations on the data.

The Pg users mailing list helped debug the disconnect between what I was trying to do and what reality was:

https://www.postgresql.org/message-id/CAOLfK3Vj-PFBJi28y1170ZP3dGeW2qpG_8_9CbaJWvEgXQ8-jQ%40mail.gmail.com

[ekohl]

I still like the hardening it provides so perhaps we should clearly document the behavior and how to deal with it if needed.

[mzagrabe]

I still like the hardening it provides so perhaps we should clearly document the behavior and how to deal with it if needed.

If we're relying on this REVOKE because bad actors are connecting to the DB and:

1. Pg < 15 - are creating objects in the DB
   or
2. Pg >= 15 - viewing the schema

...then the users of this puppet module have far greater security policies to implement. This isn't even considering that the bad actor(s) would have to be allowed access in the HBA, too.

The breakage from upstream defaults is of more concern for admins who want to leverage this module.

Either way, perhaps we can add an attribute:

Boolean $revoke_connect = true,

to the defined type. Thus allowing admins to turn off the revoking if so desired.

Thanks,

-m