-
Notifications
You must be signed in to change notification settings - Fork 18
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
Unwrap sensitive values in error messages #42
Conversation
For the record, this change is backward incompatible since matching any Sensitive value against the String |
Re-opening the PR due to age to re-kick CI. Want a fresh check on this before reviewing. |
c6b2ebf
to
7b029c4
Compare
@LukasAud I rebased on top of master hopping that this would help. The workflow need approval however. |
@smortex Sorry for the delay. Wasnt expecting the 'approve and run' function to be a blocker here. Tests are running now tho they are hitting rubocop rules. |
7b029c4
to
ce3611d
Compare
Ooops, sorry about this, should have checked locally. Rebased and fixed (rubocop and rspec seems happy on my computer). Thanks! |
@smortex This is looking good. Im happy to drop an approval but I will hold of from merging as this is a backwards-incompatible and there is a few things we want to do before cutting a major release, including cutting a minor one beforehand. Sorry for the delay but this will have to sit here for a bit longer. |
When sensitive values are compared and do not match, the produce error message does not help for debugging: ``` 1) postgresql::server::role with Password Datatype Sensitive[String] has alter role for "test" user with password as **** Failure/Error: is_expected.to contain_postgresql_psql('ALTER ROLE test ENCRYPTED PASSWORD ****') .with('command' => sensitive('ALTER ROLE "test" ENCRYPTED PASSWORD \'new-pa$s\''), 'sensitive' => 'true', 'unless' => sensitive("SELECT 1 FROM pg_shadow WHERE usename = 'test' AND passwd = 'new-pa$s'"), 'port' => '5432') expected that the catalogue would contain Postgresql_psql[ALTER ROLE test ENCRYPTED PASSWORD ****] with command set to Sensitive("ALTER ROLE \"test\" ENCRYPTED PASSWORD 'new-pa$s'") but it is set to #<Sensitive [value redacted]>, and parameter unless set to Sensitive("SELECT 1 FROM pg_shadow WHERE usename = 'test' AND passwd = 'new-pa$s'") but it is set to #<Sensitive [value redacted]> Diff: <The diff is empty, are your objects producing identical `#inspect` output?> # ./spec/defines/server/role_spec.rb:56:in `block (3 levels) in <top (required)>' # /usr/home/romain/.gem/ruby/3.0/bin/rspec:25:in `load' # /usr/home/romain/.gem/ruby/3.0/bin/rspec:25:in `<main>' ``` With this change, the sensitive values are unwrapped and allow to spot the missing unwraps in unit tests: ``` 1) postgresql::server::role with Password Datatype Sensitive[String] has alter role for "test" user with password as **** Failure/Error: is_expected.to contain_postgresql_psql('ALTER ROLE test ENCRYPTED PASSWORD ****') .with('command' => sensitive('ALTER ROLE "test" ENCRYPTED PASSWORD \'new-pa$s\''), 'sensitive' => 'true', 'unless' => sensitive("SELECT 1 FROM pg_shadow WHERE usename = 'test' AND passwd = 'new-pa$s'"), 'port' => '5432') expected that the catalogue would contain Postgresql_psql[ALTER ROLE test ENCRYPTED PASSWORD ****] with command set to Sensitive("ALTER ROLE \"test\" ENCRYPTED PASSWORD 'new-pa$s'") but it is set to Sensitive("ALTER ROLE \"test\" ENCRYPTED PASSWORD 'Sensitive [value redacted]'"), and parameter unless set to Sensitive("SELECT 1 FROM pg_shadow WHERE usename = 'test' AND passwd = 'new-pa$s'") but it is set to Sensitive("SELECT 1 FROM pg_shadow WHERE usename = 'test' AND passwd = 'Sensitive [value redacted]'") Diff: @@ -1,4 +1,4 @@ -Sensitive("ALTER ROLE \"test\" ENCRYPTED PASSWORD 'new-pa$s'") +Sensitive("ALTER ROLE \"test\" ENCRYPTED PASSWORD 'Sensitive [value redacted]'") -Sensitive("SELECT 1 FROM pg_shadow WHERE usename = 'test' AND passwd = 'new-pa$s'") +Sensitive("SELECT 1 FROM pg_shadow WHERE usename = 'test' AND passwd = 'Sensitive [value redacted]'") ```
ce3611d
to
b9dde53
Compare
👋 A new major release was cut a few days ago. I rebased my work on top of master to fix the conflicts. Can we merge and release this? |
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.
I just ran into this problem and can verify the patch helps a lot in debugging.
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.
Amazing stuff @smortex - will save a lot of debugging pain for people in the future!
When sensitive values are compared and do not match, the produce error message does not help for debugging:
With this change, the sensitive values are unwrapped and allow to spot the missing unwraps in unit tests: