Compare actual sensitive values when testing #1405
Conversation
spec/defines/server/role_spec.rb
Outdated
| 'sensitive' => 'true', | ||
| 'unless' => "SELECT 1 FROM pg_roles WHERE rolname = 'test'", | ||
| 'port' => '5432') | ||
| end | ||
| it 'has alter role for "test" user with password as ****' do | ||
| is_expected.to contain_postgresql_psql('ALTER ROLE test ENCRYPTED PASSWORD ****') | ||
| .with('command' => 'Sensitive [value redacted]', | ||
| .with('command' => sensitive(%(ALTER ROLE "test" ENCRYPTED PASSWORD 'Sensitive [value redacted]')), | ||
| # FIXME: This is obviously wrong ^^^^^^^^^^^^^^^^^^^^^^^^^^ |
There was a problem hiding this comment.
Any idea what's needed to fix this? Or is it actually a bug?
There was a problem hiding this comment.
Maybe a bug?? What harm would there be if sensitive values were unwrapped on these lines?
puppetlabs-postgresql/manifests/server/role.pp
Lines 182 to 183 in ef8129b
(given the use of
Sensitive on these? puppetlabs-postgresql/manifests/server/role.pp
Lines 186 to 187 in ef8129b
There was a problem hiding this comment.
yeah, the sensitive values definitely need unwrapping before interpolating into another string.
$secret = Sensitive('SECRET')
exec { 'test':
command => Sensitive("/bin/echo My secret is ${secret} > /tmp/test_secret"),
}cat /tmp/test_secret
My secret is Sensitive [value redacted]There was a problem hiding this comment.
Given the minimum supported version in the metadata is 6.24.0 it should be fine to just throw some unwraps in there without even checking if the value is Sensitive first.
There was a problem hiding this comment.
Yeah, it is a bug. I opened this PR in order to make it easier to diagnose the issues and fix it. For that reason I do not change the tested value in this PR so that it can hopefully be merged quickly because it does not change the current (bad) behavior. Yet, adding a comment for the obvious does not hurt.
The bug itself is tracked in #1404.
This comment was marked as resolved.
This comment was marked as resolved.
smortex
force-pushed
the
unwrap-sensitive-values-for-testing
branch
2 times, most recently
from
6045a37 to
ccae023
Compare
smortex
changed the title
smortex
force-pushed
the
unwrap-sensitive-values-for-testing
branch
from
ccae023 to
0be3db2
Compare
smortex
force-pushed
the
unwrap-sensitive-values-for-testing
branch
from
0be3db2 to
38da528
Compare
smortex
force-pushed
the
unwrap-sensitive-values-for-testing
branch
from
38da528 to
26349e0
Compare
|
@smortex is this ready to go after a rebase? |
When testing, sensitive values can be compared to the String `Sensitive [value redacted]`, but if we want to check that the redacted content is the one we expect, we should compare with another Sensitive wrapper.
smortex
force-pushed
the
unwrap-sensitive-values-for-testing
branch
from
26349e0 to
df215b2
Compare
|
@SimonHoenscheid this is ready to go! I rebased the PR, I will merge it when CI has passed. |
When testing, sensitive values can be compared to the String
Sensitive [value redacted], but if we want to check that the redacted content is the one we expect, we should compare with another Sensitive wrapper.This allows us to spot errors in the test suite (see #1404) more easily.
No functional change in this PR.