Compare decrypted values to see if they are insync #221

alexjfisher · 2019-04-06T12:26:49Z

When splunk is started, it automatically encrypts certain values when it
finds them in config files. To stop puppet reverting these changes,
I've overriden insync? so that it performs the decryption before
comparing.

Currently only implemented for splunk >= 7.2

Based on description of algorithm in this python based project.
https://github.com/HurricaneLabs/splunksecrets

alexjfisher · 2019-04-06T12:29:48Z

lib/puppet_x/voxpupuli/splunk/util.rb

+          decipher.auth_tag = tag
+          decipher.auth_data = ''
+
+          decipher.update(ciphertext) + decipher.final


Dunno if I should catch exceptions here, log a warning, and return something so that puppet can continue??

IMHO things this deep shouldn't rescue exceptions because they're too deep to know what to do about them. That should be done higher up. IMHO only rescue exceptions here if you watch to toss your own customized exception like Spunk::Util::DecodeException blah blah blah IMHO YMMV

The thought was if decryption did fail, it could be because of something like the splunk.secrets file being replaced. If decryption does fail (for whatever reason), it's maybe better for insync? to return false instead of a hard error being thrown and not handled.

I like your advice on rethrowing a custom exception and catching it higher up. I'll leave this as is in this PR, but might work on an enhancement in a separate PR later. Thanks for the tip.

jorhett · 2019-04-08T22:15:53Z

Quick question: would it be possible for Puppet to encrypt the values in a way that Splunk finds acceptable, so as to be more declarative / less subject to changes in Splunk?

jorhett · 2019-04-08T22:21:25Z

spec/acceptance/splunk_enterprise_spec.rb

  context 'default parameters' do
    # Using puppet_apply as a helper
    it 'works idempotently with no errors' do
      pp = <<-EOS
      class { '::splunk::enterprise': }
      EOS

-      # Run it twice and test for idempotency
+      # Run it three times and test for eventual idempotency (Needed for SELINUX limitations)


can you be more specific about the need, or link to something at least for the poor lass/lad who has no idea what limitation you're referring to?

Very fair point. I've removed this change as it was something I only really needed when testing locally with vagrant_libvirt and it's not otherwise related to this PR.

As for the actual issue. Puppet updates the selinux attributes on the config files on the second run since it defaults these file properties to

the value returned by matchpathcon for the file, if any exists

Puppet doesn't reread these default selinux contexts at any time during a run. Typically, they can be updated by a package being installed (eg splunk). See https://tickets.puppetlabs.com/browse/PUP-2169

Also see #153 (comment)

Update: This turned out not to be what was happening on this occasion. There is no selinux policy being installed with the RPM. Instead, on Centos 6 only (when the daemon is run from init.d not systemd), when splunk modifies the config file to encrypt values, it also ends up inadvertently modifying the seluser to unconfined_u (the context under which the process is running as). Puppet then puts it back on the next run.

When splunk is started, it automatically encrypts certain values when it finds them in config files. To stop puppet reverting these changes, I've overriden `insync?` so that it performs the decryption before comparing. Currently only implemented for splunk >= 7.2 Based on description of algorithm in this python based project. https://github.com/HurricaneLabs/splunksecrets

alexjfisher · 2019-04-09T08:49:40Z

Quick question: would it be possible for Puppet to encrypt the values in a way that Splunk finds acceptable, so as to be more declarative / less subject to changes in Splunk?

Quick question, long answer. :)

TL:DR
I've spent some time thinking about this, but think it'd actually end up being quite complicated. The algorithm itself isn't especially tricky, but the implementation could be,

Via a function...
This forces you to manage the content of the splunks.secret file (you wouldn't want to expose it as a fact.) You also need to decide how to generate the Initialisation Vector (16 bytes of data). AFAIK, it doesn't need to be random, but it does have to be unique per use. At the same time, you'd want the function to output the same value every run (maybe not required if also incorporating this change?). You could combine fqdn_rand with a seed that users supplied and put the onus on them for it being different for each value being encrypted.

In the type...
This could be better as you can generate a random IV on the fly and use the splunks.secret file that splunk generates. But the secrets file doesn't exist until the service is started, so it won't be available on the first run. If it's not there, you'd have to write the values unencrypted and by the second run, splunk will have already performed the encryption itself.

I'm not actually sure either solution would be less subject to changes in Splunk. Perhaps even the reverse. For instance, you'd still have to make very sure you were using the correct algorithm based on installed splunk version. (There are currently 2. See https://github.com/HurricaneLabs/splunksecrets).

With all that said, if anyone wants a go at implementing a function or some munging in the type, I'd be interested in seeing what they came up with. PRs welcome and all that! :)

alexjfisher added needs-tests enhancement New feature or request labels Apr 6, 2019

alexjfisher commented Apr 6, 2019

View reviewed changes

alexjfisher force-pushed the compare_values_after_decrypting branch 8 times, most recently from 8417f2a to 909ac25 Compare April 8, 2019 16:17

alexjfisher removed the needs-tests label Apr 8, 2019

bastelfreak approved these changes Apr 8, 2019

View reviewed changes

jorhett reviewed Apr 8, 2019

View reviewed changes

alexjfisher force-pushed the compare_values_after_decrypting branch from 909ac25 to 24a0da4 Compare April 9, 2019 08:22

bastelfreak merged commit f1e973e into voxpupuli:master Apr 9, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Compare decrypted values to see if they are insync #221

Compare decrypted values to see if they are insync #221

alexjfisher commented Apr 6, 2019

alexjfisher Apr 6, 2019

jorhett Apr 8, 2019

alexjfisher Apr 9, 2019 •

edited

Loading

jorhett commented Apr 8, 2019

jorhett Apr 8, 2019

alexjfisher Apr 9, 2019

alexjfisher Apr 9, 2019

alexjfisher Apr 9, 2019

alexjfisher commented Apr 9, 2019

Compare decrypted values to see if they are insync #221

Compare decrypted values to see if they are insync #221

Conversation

alexjfisher commented Apr 6, 2019

alexjfisher Apr 6, 2019

Choose a reason for hiding this comment

jorhett Apr 8, 2019

Choose a reason for hiding this comment

alexjfisher Apr 9, 2019 • edited Loading

Choose a reason for hiding this comment

jorhett commented Apr 8, 2019

jorhett Apr 8, 2019

Choose a reason for hiding this comment

alexjfisher Apr 9, 2019

Choose a reason for hiding this comment

alexjfisher Apr 9, 2019

Choose a reason for hiding this comment

alexjfisher Apr 9, 2019

Choose a reason for hiding this comment

alexjfisher commented Apr 9, 2019

alexjfisher Apr 9, 2019 •

edited

Loading