(PUP-8969) Preserve sensitiveness of epp templates

[joshcooper]

If an EPP template contains a Sensitive data type, then join the unwrapped
value, but wrap the entire result as Sensitive. This way the underlying
sensitive value is preserved, and the caller can access it by unwrapping the
entire result. Previously, the result was always a string and contained the
generic 'Sensitive [redacted]' message, so the underlying sensitive value was
lost.

Update the epp and inline_epp function signatures, since they can both
return either String or Sensitive[String].

Update the epp application to unwrap sensitive values produced by the above
functions.

[joshcooper]

This still needs tests.

Given the manifest:

$result = inline_epp("This is sensitive: <%= Sensitive.new('sesame') %>\n")

notify { 'message':
  message => $result
}

file { '/tmp/sensitive.txt':
  ensure => file,
  content => $result,
}

Previously the file contained the redacted string This is sensitive: Sensitive [value redacted]:

$ rm -f /tmp/sensitive.txt && bx puppet apply sensitive.pp && cat /tmp/sensitive.txt
Notice: Compiled catalog for localhost in environment production in 0.07 seconds
Notice: This is sensitive: Sensitive [value redacted]

Notice: /Stage[main]/Main/Notify[message]/message: defined 'message' as "This is sensitive: Sensitive [value redacted]\n"
Notice: /Stage[main]/Main/File[/tmp/sensitive.txt]/ensure: defined content as '{md5}513c3d9222613a510f8429fe6ac5c6ca'
Notice: Applied catalog in 0.01 seconds
This is sensitive: Sensitive [value redacted]

With this PR, the file contains the unwrapped value. Also note the changes to the file ensure and notify message properties:

$ rm -f /tmp/sensitive.txt && bx puppet apply sensitive.pp && cat /tmp/sensitive.txt
Notice: Compiled catalog for localhost in environment production in 0.07 seconds
Notice: Sensitive [value redacted]
Notice: /Stage[main]/Main/Notify[message]/message: changed [redacted] to [redacted]
Notice: /Stage[main]/Main/File[/tmp/sensitive.txt]/ensure: changed [redacted] to [redacted]
Notice: Applied catalog in 0.01 seconds
This is sensitive: sesame

Also if the manifest already wraps the content in sensitive:

$result = inline_epp("This is sensitive: <%= Sensitive.new('sesame') %>\n")
file { '/tmp/sensitive.txt':
  ensure => file,
  content => Sensitive.new($result),
}

Then the result is still correct (it doesn't double wrap). So I believe this is backwards compatible with existing manifests:

$ rm -f /tmp/sensitive.txt && bx puppet apply sensitive.pp && cat /tmp/sensitive.txt
Notice: Compiled catalog for localhost in environment production in 0.07 seconds
Notice: Sensitive [value redacted]
Notice: /Stage[main]/Main/Notify[message]/message: changed [redacted] to [redacted]
Notice: /Stage[main]/Main/File[/tmp/sensitive.txt]/ensure: changed [redacted] to [redacted]
Notice: Applied catalog in 0.01 seconds
This is sensitive: sesame

[puppetcla]

CLA signed by all contributors.

[justinstoller]

This seems like the desired behavior and the code looks good. I didn't see anything weird in the ast hierarchy that would require changing other classes.

I think there's an edge case if someone is putting an epp result into a typed variable where this will be backwards incompatible. eg.

String $foo = epp(...)
$final = $foo.regsubst(//, '')

However that's a weird use case (ie you should just change the template rather than acting on the content post rendering) and I'm not too concerned it changing to fix this issue.

[joshcooper]

That's a great point... and yes it fails:

Error: Evaluation Error: Error while evaluating a Method call, 'regsubst' parameter 'target' expects a value of type Array or String, got Sensitive[String] (file: /Users/josh/work/puppet/sensitive.pp, line: 2, column: 26) on node localhost

[joshcooper]

Following up after chatting with @npwalker and @reidmv. So up until now, if a sensitive value is not explicitly unwrapped in a template via Sensitive#unwrap, then the rendered text will be lossy and not really usable. People then "thrash around for a while trying to figure out that they need to call {{.unwrap}}" in the epp template, followed by wrapping the result Sensitive.new(epp(...)).

This PR eliminates the trashing around so the following just work:

file { '/tmp/app.conf':
  ensure => file,
  content => inline_epp("token: <%= Sensitive.new('opensesame') %>"),
}

This PR will is also compatible with existing manifests which unwrap and rewrap the sensitive value:

file { '/tmp/app.conf':
  ensure => file,
  content => Sensitive.new(inline_epp("token: <%= Sensitive.new('opensesame').unwrap %>")),
}

[joshcooper]

I'm not 100% about the location of these tests, but the second and third tests fail as expected when run against puppet code that doesn't have this change:

  1) the inline_epp function when using Sensitive rewraps a sensitive value
     Failure/Error:
       expect(eval_and_collect_notices(<<~END)).to eq(["Sensitive [value redacted]"])
         notice(inline_epp("This is <%= Sensitive('opensesame') %>"))
       END
     
       expected: ["Sensitive [value redacted]"]
            got: ["This is sensitive Sensitive [value redacted]"]
     
       (compared using ==)
     # ./spec/unit/functions/inline_epp_spec.rb:87:in `block (3 levels) in <top (required)>'
     # ./spec/spec_helper.rb:180:in `block (2 levels) in <top (required)>'
     # ./.bundle/ruby/2.7.0/gems/webmock-3.9.3/lib/webmock/rspec.rb:37:in `block (2 levels) in <top (required)>'

  2) the inline_epp function when using Sensitive can be double wrapped
     Failure/Error: expect(catalog.resource(:notify, 'title')['message']).to eq('opensesame')
     
       expected: "opensesame"
            got: "Sensitive [value redacted]"
     
       (compared using ==)
     # ./spec/unit/functions/inline_epp_spec.rb:98:in `block (3 levels) in <top (required)>'
     # ./spec/spec_helper.rb:180:in `block (2 levels) in <top (required)>'
     # ./.bundle/ruby/2.7.0/gems/webmock-3.9.3/lib/webmock/rspec.rb:37:in `block (2 levels) in <top (required)>'

[justinstoller]

Yeah, the location is weird, there's 4 places that these changes could be (evaluator, face, inline_epp, and epp) but to actually put something in those places is largely copying the same tests around.

[justinstoller]

This seems correct to the final request in the ticket.