Skip to content

pg_hba_rule: Validate userinput in postgresql::server #1376

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 1 commit into from
Oct 19, 2022
Merged

pg_hba_rule: Validate userinput in postgresql::server #1376

merged 1 commit into from
Oct 19, 2022

Conversation

bastelfreak
Copy link
Collaborator

No description provided.

@bastelfreak bastelfreak requested a review from a team as a code owner October 11, 2022 13:32
@bastelfreak bastelfreak self-assigned this Oct 11, 2022
@bastelfreak bastelfreak changed the title Pg hba conf pg_hba_rule: Validate userinput in postgresql::server Oct 11, 2022
@puppet-community-rangefinder
Copy link

postgresql::backup::pg_dump is a class

that may have no external impact to Forge modules.

postgresql::server is a class

Breaking changes to this file WILL impact these 39 modules (exact match):
Breaking changes to this file MAY impact these 17 modules (near match):

postgresql::server::config is a class

that may have no external impact to Forge modules.

postgresql::server::config_entry is a type

Breaking changes to this file WILL impact these 6 modules (exact match):
Breaking changes to this file MAY impact these 1 modules (near match):

postgresql::server::default_privileges is a type

that may have no external impact to Forge modules.

postgresql::server::grant is a type

Breaking changes to this file WILL impact these 2 modules (exact match):
Breaking changes to this file MAY impact these 1 modules (near match):

postgresql::server::passwd is a class

that may have no external impact to Forge modules.

postgresql::server::pg_hba_rule is a type

Breaking changes to this file WILL impact these 14 modules (exact match):
Breaking changes to this file MAY impact these 4 modules (near match):

postgresql::server::role is a type

Breaking changes to this file WILL impact these 22 modules (exact match):
Breaking changes to this file MAY impact these 6 modules (near match):

postgresql::validate_db_connection is a type

Breaking changes to this file WILL impact these 3 modules (exact match):
Breaking changes to this file MAY impact these 2 modules (near match):

This module is declared in 70 of 579 indexed public Puppetfiles.

These results were generated with Rangefinder, a tool that helps predict the downstream impact of breaking changes to elements used in Puppet modules. You can run this on the command line to get a full report.

Exact matches are those that we can positively identify via namespace and the declaring modules' metadata. Non-namespaced items, such as Puppet 3.x functions, will always be reported as near matches only.

tuxmea
tuxmea reviewed Oct 11, 2022
View reviewed changes
@bastelfreak bastelfreak force-pushed the pg_hba_conf branch 10 times, most recently from 3f5a38e to 22cc951 Compare October 11, 2022 15:30
@bastelfreak
Copy link
Collaborator Author

@david22swan I rebased this one as well.

@david22swan
Copy link
Member

@bastelfreak Look's like this one has a few syntax failures

@bastelfreak
Copy link
Collaborator Author

@david22swan should be good to go now

@bastelfreak bastelfreak force-pushed the pg_hba_conf branch 3 times, most recently from f92c6e9 to fb4500b Compare October 11, 2022 15:54
tuxmea
tuxmea previously approved these changes Oct 12, 2022
View reviewed changes
smortex
smortex reviewed Oct 12, 2022
View reviewed changes
manifests/server.pp
Hash[String, Hash] $pg_hba_rules = {},
Hash[String, Hash] $roles = {},
Hash[String, Any] $config_entries = {},
Postgresql::Pg_hba_rules $pg_hba_rules = {},
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Given the default value is an empty Hash…

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

empty hashes should still pass. types for hashes only match when there's data in it ( that's why we recommend at vox pupuli that the default value should always be an empty hash, not undef). I added a spec test for empty hashes. let's see if it passes or if I wrote bullshit :D

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ooops, read to fast: here we have Postgresql::Pg_hba_rules which is a Hash[String, Struct] and should accept an empty Hash.

I was speaking about Postgresql::Pg_hba_rule (no s) which is a Struct and should validate that all non-optional members are set.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

An empty hash also gets evaluated with the data type. Therefor all keys must be optional (keys! not values!) to allow passing an empty Struct.

Optional['description'] => String,
...

types/pg_hba_rule.pp Outdated
Comment on lines 4 to 13
description => String,
type => Postgresql::Pg_hba_rule_type,
database => String,
user => String,
Optional[address] => Optional[Postgresql::Pg_hba_rule_address],
auth_method => String,
Optional[auth_option] => Optional[String],
order => Variant[String,Integer],
target => Stdlib::Absolutepath,
postgresql_version => String,
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

…all keys should be Optional I guess.

@bastelfreak bastelfreak force-pushed the pg_hba_conf branch 2 times, most recently from 47900d2 to 73f90f6 Compare October 12, 2022 07:26
@bastelfreak bastelfreak force-pushed the pg_hba_conf branch 4 times, most recently from b333eeb to d18a9f1 Compare October 12, 2022 08:07
tuxmea
tuxmea reviewed Oct 12, 2022
View reviewed changes
types/pg_hba_rule.pp
# @see https://github.com/puppetlabs/puppetlabs-postgresql/blob/main/manifests/server/pg_hba_rule.pp
type Postgresql::Pg_hba_rule = Struct[{
Optional[description] => String,
type => Postgresql::Pg_hba_rule_type,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also optional - to be more precise: If an empty hash is OK, all keys must be Optional.

@bastelfreak bastelfreak force-pushed the pg_hba_conf branch 2 times, most recently from 5d7a536 to 1e84c7d Compare October 12, 2022 08:44
@bastelfreak
Copy link
Collaborator Author

@david22swan can you take a look again please? :)

david22swan
david22swan approved these changes Oct 19, 2022
View reviewed changes
Copy link
Member

@david22swan david22swan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@david22swan david22swan merged commit 7860557 into puppetlabs:main Oct 19, 2022
@david22swan
Copy link
Member

Look's like a good change :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@smortex smortex smortex left review comments

@david22swan david22swan david22swan approved these changes

+1 more reviewer

@tuxmea tuxmea tuxmea left review comments

Reviewers whose approvals may not affect merge requirements
Assignees

@bastelfreak bastelfreak

Labels
community feature
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@bastelfreak @david22swan @smortex @tuxmea