Add sensitive flag to resources to restrict logging output

[arothian]

Intending this as a starting point for a discussion on how this might best work. In Chef proper, there is a common attribute 'sensitive' that is used to suppress various output that might be logged, particularly during failures. I think the same use case can apply to chef-inspec. In particular, when using the file resource, if you are checking a sensitive file for specific content, you may not want the expected vs actual output to be logged.

This concept adds the same sensitive flag as a base attribute on inspec resources and modifies the formatters to filter out message output if it is set on the resource. What's missing here are some good tests, and I'm curious what thoughts might be for how to mark a resource as sensitive. I added one possibility related to the file resource.

Signed-off-by: Kevin Formsma kevin.formsma@gmail.com

[chris-rock]

@arothian It is a great idea to make information more secure and not expose internal information on CLI or logs that do not belong there. Can you please add some specific cases, where sensitive information is exposed? Shouldn't we expect that each resource knows what is sensitive and what not? How is the resource argument helping to reduce that?

[chris-rock]

I feel that we may do not want to add an additional option to each resource. This adds additional burden and makes resource development more complex.

[adamleff]

I'm very 👍 on this idea, and it comes up relatively frequently. I agree with @chris-rock though that this shouldn't be an argument to the resource itself. I'd rather see the DSL extended similar to how Chef does this with properties:

describe file('/etc/shadow') do
  sensitive true
  its('content') { should match 'blah' }
end

[adamleff]

To @chris-rock's point, I don't think it's reasonable for each resource to know what's sensitive. i.e. the burden on the file resource to know which files are sensitive and which ones aren't is extremely large. I'd rather put control in the hands of the users for something like this.

[arothian]

Thanks for the feedback.

@adamleff, I like the suggested format for setting the flag.

I agree that it doesn't make sense for a resource to know what may be sensitive. It should be up to the user to mark as such.

For me, the specific use case is around file resources and the its('content') matchers. I'm creating a check that a configuration file contains the required keys, and that config also has sensitive data. If the check fails, currently the output exposes that file information. There are probably other use cases.

I'll see about updating this PR with the suggested way of setting the property

[arothian]

@adamleff, @chris-rock
I updated this with another potential solution for configuring the proposed sensitive flag. I didn't see a straightforward way to add it within the block passed to the rspec example. However, we can use rspec's default functionality for specifying boolean user metadata and then filtering on that it the formatter. Let me know what you think. The direct metadata route, should bypass any complexity with resource development.

I didn't see any unit tests related to the formatters or a easy way to incorporate this into the existing integration tests. If you have an idea there, let me know. The documentation would need to get updated yet.

Here is what I was using to test with. The output from the first resource should be suppressed.
echo "sensitive" > /tmp/example

describe file('/tmp/example'), :sensitive do
  its('content') { should match(/bob/) }
end

describe file('/tmp/example') do
  its('content') { should match(/bob/) }
end

[adamleff]

@arothian I apologize for the delay in our response! I think this is a really neat solution and totally acceptable until we have a better way of extending the describe DSL.

Here's what needs to happen next:

• another maintainer needs to approve. I'd like @chris-rock to weigh in on this
• I'd like to see a test added for this new behavior. Unit testing the RSpec formatter is a path to sadness, so I'd recommend adding a functional test (see test/functional for examples). Please let me know if you have any questions or need suggestions on how to proceed
• We need you to sign-off on your commits per the DCO

Thank you for your contribution thus far!

[arothian]

@adamleff Thanks for the feedback. I've added the mentioned functional tests for the new functionality, took a stab at updating the docs.

I rebased on the latest from master.

@chris-rock Should be ready to take a look at.

[adamleff]

@arothian this looks great! I have one suggestion on the sensitive output that I'd love to see updated, but I'm not going to block this change on that request. But please update if it you agree and you have a moment.

[adamleff]

Can I suggest we change this to *** sensitive output suppressed *** to make it extra clear?

[arlimus]

Great idea an approach, thank you @arothian !!

[adamleff]

Pushed up a small change to fix the test output as a result of #2094 getting merged before this. The color control characters around "0 skipped" are no longer necessary as we only colorize the output if the number is > 0.

Once this goes green in Travis, I'll merge!

[adamleff]

Merging! Thanks again, @arothian!

[dialex]

Hi @adamleff, does this feature apply to the bash resource?

I have an inspec test that calls an API via curl with an authentication token. Currently the inspec output shows the whole curl command (including the auth token) and I need to hide it.

describe bash(command_with_sensitive_data), :sensitive do
    its('stdout') { should match "200 OK" }
  end

Using the `:sensitive" flag did not hide the command. Using an env var is not enough, as inspec resolves it before outputting the command.

[adamleff]

The sensitive flag only hides output from the commands/diffs, but not the actual command.

In your case, you'll likely want to the expect syntax which allows you to have a bit more control over the output:

describe "the website" do
  subject { bash(command_with_sensitive_data) }
  it "should return a 200 OK status" do
    expect(subject.stdout).to include("200 OK")
  end
end

[dialex]

Oh, that works perfectly.

I wasn't aware of that syntax! Do you have any examples on Inspec's documentation? I don't remember seeing it. Where can I read more about it? For instance, is there any other matcher besides includes? Does it accept a regex?

Is there any reason when to use one syntax or the other?

I was actually going to "request a feature" to customise the output of test steps, to avoid printing long regex, or to make them more readable than the automatically generated message. I think this can do the trick...

[adamleff]

@dialex I recently added an example of the "expect" syntax on the website: https://www.inspec.io/docs/reference/profiles/#should-vs-expect-syntax

Regarding additional matchers, we document some of them on our site, and you can also look through additional RSpec examples at sites like http://www.rubydoc.info/gems/rspec-expectations/frames ... again, we don't advocate for much of the advanced use as one of our primary goals is to remain readable to non-technical users. That said, there's nothing stopping you from going off the beaten path. 🙂