[PUP-9295] Notify resource exposes Sensitive data when message is a Sensitive data Created: 2018/10/31  Updated: 2019/11/19  Resolved: 2019/10/09

Status: Resolved
Project: Puppet
Component/s: Docs, Types and Providers
Affects Version/s: None
Fix Version/s: PUP 6.10.1, PUP 5.5.18, PUP 6.4.5

Type: Bug Priority: Major
Reporter: Kevin Reeuwijk Assignee: Josh Cooper
Resolution: Fixed Votes: 0
Labels: resolved-issue-added
Remaining Estimate: Not Specified
Time Spent: Not Specified
Original Estimate: Not Specified

Issue Links:
Duplicate
Relates
relates to PUP-10092 Support concatenating sensitive values Accepted
relates to PUP-7580 Exec resource exposing Sensitive data Closed
Template: PUP Bug Template
Epic Link: Redacting Sensitive Data
Team: Coremunity
Sprint: Platform Core KANBAN
Method Found: Needs Assessment
Release Notes: Bug Fix
Release Notes Summary: Redact sensitive values when interpolated in a notify resource's message.

note added for 6.10.1-jb
QA Risk Assessment: Needs Assessment

 Description   

Puppet Version: 6.0.2
Puppet Server Version: 2019.0.0
OS Name/Version: Mac OS X, also confirmed on Linux by customer

The Notify{} resource will leak Sensitive data if the message is a raw value of Sensitive() type.

It looks like the Notify{} resource is mishandling the processing of the message parameter if the input is a raw Sensitive datatype (not encapsulated in double quotes).

Desired Behavior:

No Sensitive data should be output in clear text.

Actual Behavior:

For example, in the following code both message 1 and 2 leak sensitive data:

$secret = Sensitive('s3cret')
notify { 'Sensitive message 1':
 message => $secret,
}
notify { 'Sensitive message 2':
 message => Sensitive('s3cret2'),
}
notify { 'Sensitive message 3':
 message => "${secret}",
}

Output:

Notice: Compiled catalog for kevin.reeuwijk-c02tp0a7g8wl in environment production in 0.03 seconds
Notice: s3cret
Notice: /Stage[main]/Main/Notify[Sensitive message 1]/message: changed [redacted] to [redacted]
Notice: s3cret2
Notice: /Stage[main]/Main/Notify[Sensitive message 2]/message: changed [redacted] to [redacted]
Notice: Sensitive [value redacted]
Notice: /Stage[main]/Main/Notify[Sensitive message 3]/message: defined 'message' as 'Sensitive [value redacted]'
Notice: Applied catalog in 0.02 seconds

There are two messages in the output that should not appear:

 

Notice: s3cret
Notice: s3cret2



 Comments   
Comment by Josh Cooper [ 2019/01/14 ]

The Puppet::Pops::Types::PSensitiveType#to_s method is only called in the third case, I assume as a result of interpolating the sensitive message in a string.

One fix is to do the following, but it feels like a game of "whack a mole" as every type/provider needs to check if it's been given a sensitive property/parameter:

diff --git a/lib/puppet/type/notify.rb b/lib/puppet/type/notify.rb
index 96830aa91b..aa7cb5603b 100644
--- a/lib/puppet/type/notify.rb
+++ b/lib/puppet/type/notify.rb
@@ -11,11 +11,12 @@ module Puppet
     newproperty(:message, :idempotent => false) do
       desc "The message to be sent to the log."
       def sync
+        message = @sensitive ? 'Sensitive [value redacted]' : self.should
         case @resource["withpath"]
         when :true
-          send(@resource[:loglevel], self.should)
+          send(@resource[:loglevel], message)
         else
-          Puppet.send(@resource[:loglevel], self.should)
+          Puppet.send(@resource[:loglevel], message)
         end
         return
       end

$ bundle exec puppet apply manifest.pp
Notice: Compiled catalog for localhost in environment production in 0.02 seconds
Notice: Sensitive [value redacted]
Notice: /Stage[main]/Main/Notify[Sensitive message 1]/message: changed [redacted] to [redacted]
Notice: Sensitive [value redacted]
Notice: /Stage[main]/Main/Notify[Sensitive message 2]/message: changed [redacted] to [redacted]
Notice: Sensitive [value redacted]
Notice: /Stage[main]/Main/Notify[Sensitive message 3]/message: defined 'message' as 'Sensitive [value redacted]'
Notice: Applied catalog in 0.01 seconds

Comment by Henrik Lindberg [ 2019/01/31 ]

I don't see how we can get around this for types/providers that do their own logging/output where values should be appear as redacted without actually making them recognize if they should redact. This because they are written to operate on a single clear text value.

Comment by Josh Cooper [ 2019/03/01 ]

But Henrik Lindberg why does interpolating the secret value correctly redact:

$ bx puppet apply -e "\$secret = Sensitive.new('foo'); notify { 'n': message => \"\${secret}\"}"
Notice: Compiled catalog for localhost in environment production in 0.03 seconds
Notice: Sensitive [value redacted]

But not when handled the secret value directly:

$ bx puppet apply -e "\$secret = Sensitive.new('foo'); notify { 'n': message => \$secret}"
Notice: Compiled catalog for localhost in environment production in 0.03 seconds
Notice: foo

Surely providers shouldn't be expected to handle those two cases differently?

Comment by Henrik Lindberg [ 2019/03/01 ]

Because when the sensitive value is interpolated that takes place when the catalog is compiled and a Sensitive value turns it into a string by generating "redacted". When doing this the agent gets the string "redacted" not a sensitive value - and this is probably not what the user intended, so there really is no "other case" for a provider to deal with.

The support for sensitive on the agent side is partly in the harness - reporting values etc. checks if param is marked as sensitive. All types providers that do something else with values must be aware that a value may be sensitive and handle it appropriately. This is naturally not great.

The alternative is not great either: we could instead of setting the sensitive flag in the resource have the values actually be of type Sensitive. Then, any type provider getting this value would need to know how to unwrap it - and thus also be aware that it is handling a sensitive value. That has other issues naturally - and this was why the design with a separate sensitive flag was introduced. (If we had done this, it would be more secure, but would be a lot more painful).

Comment by Jorie Tappa [ 2019/06/12 ]

If it's an intended behavior, we should be explicit about it in the docs. If it's not, we can prioritize this for future Sensitive improvements. If it's an intended behavior that we now want to change, we need approval/priority from Products or a PM.

Comment by Kris Bosland [ 2019/07/15 ]

The team is coming to the view that the least surprise for users is if only code specifically designed to handle Sensitive values can get the unredacted version.  It should not show up in even Debug logs by default, because that requires our users to manage their debug logs and reports as Sensitive data.  This is a larger scale engineering effort, and we will work towards tickets and epics to outline this work.

Comment by Jorie Tappa [ 2019/07/22 ]

Regardless of original intentions, we do not want to reveal any sensitive data in the notify resource. Josh Cooper's comment above should resolve this for the notify resource specifically, but there might be other issues we haven't come across yet. 

 

As Henrik mentioned above, we could've implemented another flag to force a reveal but we haven't, so redacting the data should always be the expected behavior, and the safest/least surprise for the user.

Comment by Josh Cooper [ 2019/10/04 ]

There are two issues going on. This ticket is about the notify resource printing messages 1 and 2 to the console, which ends up logs. The second issue (filed as PUP-10092) is that the compiler evaluates these differently, which is surprising UX:

$secret = Sensitive('secret')
notify { 'a': message => $secret }
notify { 'b': message => "${secret}" }

The resulting catalog contains:

    {
      "type": "Notify",
      "title": "a",
      "tags": [
        "notify",
        "a",
        "class"
      ],
      "file": "/etc/puppetlabs/code/environments/production/manifests/site.pp",
      "line": 5,
      "exported": false,
      "parameters": {
        "message": "secret"
      },
      "sensitive_parameters": [
        "message"
      ]
    },
    {
      "type": "Notify",
      "title": "b",
      "tags": [
        "notify",
        "b",
        "class"
      ],
      "file": "/etc/puppetlabs/code/environments/production/manifests/site.pp",
      "line": 6,
      "exported": false,
      "parameters": {
        "message": "Sensitive [value redacted]"
      }
    }

The compiler preserves the sensitive value for resource "a" and marks it as sensitive, while resource "b" is coverted to the string "Sensitive [value redacted]", which is lossy.

Comment by Josh Cooper [ 2019/10/04 ]

Merged to 5.5.x in https://github.com/puppetlabs/puppet/commit/679d55489b1d1c13467272d647feb05e1065fbf6

Comment by Josh Cooper [ 2019/10/04 ]

This did not make 5.5.17, but should make 6.4.4 and 6.10.1

Comment by Kris Bosland [ 2019/10/07 ]

Merged to 5.5.x in 7781997.

Comment by George Mrejea [ 2019/10/09 ]

I believe this can be closed since the commit was promoted and tested in master.

Comment by Josh Cooper [ 2019/10/09 ]

Passed CI in 36c35404b8474b5949c87c49e0b360d426b64cbb

Comment by Josh Cooper [ 2019/11/19 ]

Ciprian Badescu, Heston Hoffman This was fixed in 6.10.1, it doesn't need to be "fixed" again in 6.11.0

Generated at Sat Sep 19 12:24:03 PDT 2020 using Jira 8.5.2#805002-sha1:a66f9354b9e12ac788984e5d84669c903a370049.