Skip to content

Fix wrong Sensitive handling for updating role passwords #1404

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

Merged
merged 1 commit into from
Jul 12, 2023
Merged

Fix wrong Sensitive handling for updating role passwords #1404

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion manifests/server/role.pp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -166,7 +166,7 @@
$pwd_hash_sql = postgresql::postgresql_password(
$username,
$password_hash,
Copy link
Collaborator

Choose a reason for hiding this comment

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

Calling postgresql::postgresql_password() with a variable $password_hash as the password parameter look wrong: as far as I can see, postgresql::server::role only has a password_hash parameter, so I would not expect it to hash anything itself (and be passed an already hashed password).

The postgresql::server::db class has a password parameter which is copied to the password_hash parameter of postgresql::server::role verbatim. This is inconsistent, but I think this is for backward compatibility since when I check my (old) code I have things like:

postgresql::server::db { $taiga::back::db_name:
  user     => $taiga::back::db_user,
  password => postgresql_password($taiga::back::db_user, $taiga::back::db_password),
}

The test suite mostly checks that values are sensitive ones, so basically that they match Sensitive [value redacted], but not the actual unwrapped value. I opened a PR to unwrap Sensitive values when running the test suite and as you guessed on Slack, some content seems to be wrapped twice.

Copy link
Contributor Author

@cruelsmith cruelsmith Feb 25, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

Yes, the hashing of a hash was also my first guess but it works since it returns the same value as long as the hashtype and salt does match.
Already have an idea to do a check like that to prevent duplicate hashing:

unless $password_hash_unsensitive =~ /^md5[0-9a-f]{32}$/ and $password_hash_unsensitive =~ /^scram-sha-256:/ { 
  $password_sql = postgresql_password([...])
} else {
  $password_sql = $password_hash_unsensitive
}

But i would say i create an additional PR since this PR is bug fixing and the duplicate hashing is more or less a maintenance change.

Edit: The function postgresql_password itself checks for the hash already and then just return the password as it is. But it will handle a Sensitive password hash as a password that needs to be hashed and does not convert between Sensitive and not Sensitive with third parameter expect to do.

puppetlabs-postgresql/lib/puppet/functions/postgresql/postgresql_password.rb

Lines 31 to 33 in 7ee8e5c

if password.is_a?(String) && password.match?(%r{^(md5|SCRAM-SHA-256).+})
return password
end

Copy link
Collaborator

Choose a reason for hiding this comment

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

Or just edit lines 182 and 183??

eg.

$pw_command = "ALTER ROLE \"${username}\" ENCRYPTED PASSWORD '${pwd_hash_sql}'"

becomes

$pw_command = "ALTER ROLE \"${username}\" ENCRYPTED PASSWORD '${pwd_hash_sql.unwrap}'"

Copy link
Collaborator

Choose a reason for hiding this comment

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

From puppet 6.24.0 onwards, using unwrap on non Sensitive strings is fine.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sure but then we should also update the Deferred way to use Sensitive and rework to remove this, or?

puppetlabs-postgresql/manifests/server/role.pp

Lines 45 to 49 in ad330fc

$password_hash_unsensitive = if $password_hash =~ Sensitive[String] {
$password_hash.unwrap
} else {
$password_hash
}

Copy link
Collaborator

Choose a reason for hiding this comment

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

That part predates the Puppet 6.24 minimum version bound and can indeed be simplified.

$password_hash =~ Sensitive[String],
false,
$hash,
$salt,
)
Expand Down
6 changes: 2 additions & 4 deletions spec/defines/server/role_spec.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -58,11 +58,9 @@

it 'has alter role for "test" user with password as ****' do
expect(subject).to contain_postgresql_psql('ALTER ROLE test ENCRYPTED PASSWORD ****')
.with('command' => sensitive(%(ALTER ROLE "test" ENCRYPTED PASSWORD 'Sensitive [value redacted]')),
# FIXME: This is obviously wrong ^^^^^^^^^^^^^^^^^^^^^^^^^^
.with('command' => sensitive(%(ALTER ROLE "test" ENCRYPTED PASSWORD 'md5b6f7fcbbabb4befde4588a26c1cfd2fa')),
'sensitive' => 'true',
'unless' => sensitive(%(SELECT 1 FROM pg_shadow WHERE usename = 'test' AND passwd = 'Sensitive [value redacted]')),
# FIXME: This is obviously wrong ^^^^^^^^^^^^^^^^^^^^^^^^^^
'unless' => sensitive(%(SELECT 1 FROM pg_shadow WHERE usename = 'test' AND passwd = 'md5b6f7fcbbabb4befde4588a26c1cfd2fa')),
'port' => '5432')
end
end
Expand Down