Support sensitive values in to_json_pretty
#1418
Conversation
| call_function('stdlib::rewrap_sensitive_data', data) do |unwrapped_data| | ||
| # Call ::JSON to ensure it references the JSON library from Ruby's standard library | ||
| # instead of a random JSON namespace that might be in scope due to user code. | ||
| ::JSON.pretty_generate(unwrapped_data, opts) << "\n" |
There was a problem hiding this comment.
Leading colons put back in to match existing comment. See #1307 (comment)
|
@binford2k Would be interested to hear your thoughts around this. I dunno if I've come up with a great or terrible idea! :) |
|
I like the idea behind this. Looks like a nice QoL update to clean up functions and make things a bit more readable 👍 Unfortunately, I can't tell right away if there is any limitations or issues with this approach but I'll try to shine some light on this PR and see if it catches some eyes. |
| obj.each_with_object({}) do |(key, value), result| | ||
| result[key] = deep_unwrap(value) | ||
| end |
There was a problem hiding this comment.
Probably unlikely that you want to unwrap keys, so you can also use transform_values for shorter code:
| obj.each_with_object({}) do |(key, value), result| | |
| result[key] = deep_unwrap(value) | |
| end | |
| obj.transform_values { |value| deep_unwrap(value) } |
There was a problem hiding this comment.
Thanks. If anyone ever does come up with a situation where they've ended up with a sensitive(string) as a hash key we can always change it back and add this functionality.
There was a problem hiding this comment.
I've had a rethink! :) I agree it's probably unlikely to come across sensitive keys, but not impossible or inherently wrong. It's also easy enough to code for, so I've added a test and now handle this case.
|
|
||
| unwrapped = deep_unwrap(data) | ||
|
|
||
| result = block_given? ? yield(unwrapped) : unwrapped |
There was a problem hiding this comment.
Should the yielded unwrapped actually return a Sensitive. Or in other words, should it be yield(wrapped) instead?
There was a problem hiding this comment.
No, I think this is correct. The block, (if provided), needs to be handed the unwrapped data so that it can mutate it as desired (eg. serialising to yaml or json), before this function then wraps the result back up with sensitive.
I've added the first spec tests that hopefully illustrates this a bit better.
alexjfisher
force-pushed
the
why_so_sensitive
branch
4 times, most recently
from
6a51eed
to
1f22947
Compare
The idea is being able to do something like this, (where just the call
to `to_json_pretty` is explicitly `Deferred` - needed because
`node_encrypt` returns a `Deferred`)
```puppet
file { '/etc/my_secret.json':
content => Deferred('to_json_pretty',[{
username => 'myuser',
password => lookup('my_eyaml_secret').node_encrypt::secret,
}]),
}
```
... instead of having to also explicitly defer `unwrap` and `Sensitive` and
end up with a huge mess similar to...
```puppet
file { '/etc/my_secret.json':
content => Sensitive(
Deferred('to_json_pretty',[{
username => 'myuser',
password => Deferred('unwrap', [lookup('my_eyaml_secret').node_encrypt::secret]),
}])
),
}
```
The thought behind `rewrap_sensitive_data` is it makes it easy to
extend this functionality into other similar functions, (`to_yaml`,
`to_toml` etc.)
Later, we might consider adding a `deferrable_to_XXX` functions to
further simplify this sort of use-case.
alexjfisher
force-pushed
the
why_so_sensitive
branch
from
1f22947
to
e7fa675
Compare
alexjfisher
changed the title
to_json_prettyto_json_pretty
alexjfisher
requested review from
b4ldr,
bastelfreak,
smortex,
seanmil and
a team
as code owners
|
I really like the rewrap-with-a-block idea. I hate that we have to do it. 🤣 |
The idea is being able to do something like this, (where just the call to
to_json_prettyis explicitlyDeferred- needed becausenode_encryptreturns aDeferred)... instead of having to also explicitly defer
unwrapandSensitiveand end up with a huge mess similar to...The thought behind
rewrap_sensitive_datais it makes it easy to extend this functionality into other similar functions, (to_yaml,to_tomletc.)Later, we might consider adding a
deferrable_to_XXXfunctions to further simplify this sort of use-case.No spec tests yet. Just wanted to get some early feedback before taking it further.