+1 on getting this merged. It really hurts community involvement when PRs take this long to get accepted.

Apologies for delaying this. We will look into this as soon as we release RC3

@tejaycar thank you for bringing this PR to our attention - we get so many, sometimes things fall behind.

I did a quick glance at this PR, and I don't see any tests. @tejaycar did you test this? What were the results?

We don't have any cryptographers on the team, so it may be difficult to correctly evaluate the changes being made here. While I agree some of the behaviors (IV usage, for example) today are really bad bugs, I don't know if I'm strong enough in crypto to evaluate the proposed change with respect to cryptographic strength.

@jordansissel honestly, I'm probably less qualified than you are with regards to the strength of this one. Frankly, I'd far rather see a cipher codec, but I'm in no position to make that happen. And no, I have not tested it. I'm not even sure how to test it without having it released so I can install the additions. But thanks for the quick attention. Hopefully the original contributor can help with any concerns you may have.

I'm not even sure how to test it without having it released so I can install the additions

This is a good point! We'll work on documentation for helping folks more easily test patches (for emergency production fixes or just generally providing feedback on patches).

<3

I've been running this code (previous patch I submitted in the old project which is in this PR) in production for over a year to encrypt all of our events prior to dumping to redis. Would be great if it could be moved along.

I filed bug #6 and can confirm this PR resolves it. Would love to see this merged and a new release published.

+1 Are we going to see this anytime soon? This PR has been open for over a year, and currently we really don't have any good options for encrypted log transport without it.

@tejaycar

Are we going to see this anytime soon

My previous comment still stands about not having specific cryptographic experience to review this effectively. The code may execute, but is it safe enough? I don't know.

we really don't have any good options for encrypted log transport without it.

The lumberjack output uses SSL for encryption and is used for transmitting logs. Another alternative is to ship in plain text over an encrypted network layer such as IPSec. I'm not saying these are the only options, just saying that there _are_options that are not this plugin.

I want to make clear that I"m not trying to be a pessimist or some kind of blockade. Security is a tricky area where the code can appear to work but have very bad side effects that destroy the safety assumptions of those using them.

If you like this patch but cannot help evaluate the safety of it, that's ok! You have some options. This PR and this plugin are open source, you can fork it and pull the patch in yourself and publish your own fork of the plugin. I'm not saying you have to fork, but you can certainly do so.

I can tell you one thing, this plugin as it stands today without this PR merged, is far less secure as it (a) has bugs and (b) encourages the usage of a hardcoded static IV...

Bump, we are now looking to upgrade to Logstash 1.5.x.... when is this going to be evaluate for inclusion in to the main line?

Jenkins standing by to test this. If you aren't a maintainer, you can ignore this comment. Someone with commit access, please review this and clear it for Jenkins to run; then say 'jenkins, test it'.

Approaching one year since this PR was submitted.... seriously what is the delay with this?

@bitsofinfo Thank you for the ping. Apologies for not getting to this sooner.

We can probably make you the maintainer of this community-supported plugin, if you're interested. If you're interested, please read the community maintainer guide. Let us know if this is something you're interested in.

Thanks @untergeek, appreciate the offer however I really can't dedicate the time to be the maintainer of this going forward w/ new features etc. I am just really asking that these improvements be brought into the mainline release so that my team can stop having to maintain a patched version of this plugin in our puppet deployments for logstash. (as I am sure other folks are doing now as well).

I can't speak for others, but we have been using this PR patched version in 1.4.x and 1.5.x for over a year now for encrypting fields. Works fine (for us) perhaps others can chime in.

@bitsofinfo no worries!

In order to merge this, the version needs to be bumped in the gemspec file, and a short blurb should be added to the changelog file.

@untergeek done

The absence of tests is concerning. The code looks good to me, but I'd really like to see rspec tests to validate code.

yes, and tests have been absent since this plugin's inception, prior to this PR

You must understand our position here. We simply cannot blindly merge plugins without tests any more, regardless of whether they were omitted in the past. New plugin submissions require tests, and updates require tests. We need to be able to run continuous integration testing against plugins in the logstash-plugins repositories.

We are working on adding tests to plugins which do not have them, currently. However, with limited team resources we have to prioritize core Logstash issues, new features, and plugins which are more widely used. This plugin has slipped through the cracks, for which I've apologized, but I can't promise it will be getting tests from the Logstash team in any kind of time frame. This is why the offer to have you become a community maintainer of this plugin was extended. You're clearly passionate about it, and want to keep using it. We rely on community members to drive forward the plugins they're passionate about because we don't yet have enough team members to cover every plugin with the attention they deserve.

relax; please point me to a plugin that has some tests as and example, so I can create some for this.

https://github.com/logstash-plugins/logstash-filter-mutate/blob/master/spec/filters/mutate_spec.rb is a good example. Please use expect().to syntax for RSpec 3, instead of the older insist syntax (which we're pruning out as quickly as we can).

Ok, I added a few

Thanks for the tests. Sorry for the delay. I'm traveling right now. Will get to this soon.

?

I am so sorry for dropping the ball here. I was in Japan when I made my last comment, and a lot fell through the cracks after that trip. The tests and code LGTM. Let me get another quick look at this from a co-worker and we'll merge.

http://stackoverflow.com/questions/6481627/java-security-illegal-key-size-or-default-parameters
getting some errors coming from java:

% java -version
java version "1.8.0_20"
Java(TM) SE Runtime Environment (build 1.8.0_20-b26)
Java HotSpot(TM) 64-Bit Server VM (build 25.20-b23, mixed mode)

 1) LogStash::Filters::Cipher 1000 events, 11 re-use, encrypt/decrypt aes-256-cbc, 16b RANDOM IV, 32b key, b64 encode validate decrypted message
     Failure/Error: pipeline.filter(evt) do |filtered_event|
     OpenSSL::Cipher::CipherError:
       java.security.InvalidKeyException: Illegal key size: possibly you need to install Java Cryptography Extension (JCE) Unlimited Strength Jurisdiction Policy Files for your JRE
     # ./lib/logstash/filters/cipher.rb:209:in `init_cipher'
     # ./lib/logstash/filters/cipher.rb:185:in `filter'

.....

 4) LogStash::Filters::Cipher 1000 events, 11 re-use, encrypt/decrypt aes-256-cbc, 16b RANDOM IV, 32b key, b64 encode validate initial cleartext message
     Failure/Error: pipeline.filter(evt) do |filtered_event|
     OpenSSL::Cipher::CipherError:
       java.security.InvalidKeyException: Illegal key size: possibly you need to install Java Cryptography Extension (JCE) Unlimited Strength Jurisdiction Policy Files for your JRE
     # ./lib/logstash/filters/cipher.rb:209:in `init_cipher'
     # ./lib/logstash/filters/cipher.rb:185:in `filter'

actually all seem to fail for me:

rspec ./spec/filters/cipher_spec.rb:200 # LogStash::Filters::Cipher 1000 events, 11 re-use, encrypt/decrypt aes-256-cbc, 16b RANDOM IV, 32b key, b64 encode validate decrypted message
rspec ./spec/filters/cipher_spec.rb:206 # LogStash::Filters::Cipher 1000 events, 11 re-use, encrypt/decrypt aes-256-cbc, 16b RANDOM IV, 32b key, b64 encode validate encrypted message is not equal to message
rspec ./spec/filters/cipher_spec.rb:190 # LogStash::Filters::Cipher 1000 events, 11 re-use, encrypt/decrypt aes-256-cbc, 16b RANDOM IV, 32b key, b64 encode validate total events
rspec ./spec/filters/cipher_spec.rb:194 # LogStash::Filters::Cipher 1000 events, 11 re-use, encrypt/decrypt aes-256-cbc, 16b RANDOM IV, 32b key, b64 encode validate initial cleartext message
rspec ./spec/filters/cipher_spec.rb:137 # LogStash::Filters::Cipher single event, encrypt/decrypt aes-256-cbc, 16b STATIC IV, 32b key, b64 encode validate encrypted message is not equal to message
rspec ./spec/filters/cipher_spec.rb:132 # LogStash::Filters::Cipher single event, encrypt/decrypt aes-256-cbc, 16b STATIC IV, 32b key, b64 encode validate decrypted message
rspec ./spec/filters/cipher_spec.rb:127 # LogStash::Filters::Cipher single event, encrypt/decrypt aes-256-cbc, 16b STATIC IV, 32b key, b64 encode validate initial cleartext message
rspec ./spec/filters/cipher_spec.rb:84 # LogStash::Filters::Cipher single event, encrypt/decrypt aes-256-cbc, 16b RANDOM IV, 32b key, b64 encode validate encrypted message is not equal to message
rspec ./spec/filters/cipher_spec.rb:79 # LogStash::Filters::Cipher single event, encrypt/decrypt aes-256-cbc, 16b RANDOM IV, 32b key, b64 encode validate decrypted message
rspec ./spec/filters/cipher_spec.rb:74 # LogStash::Filters::Cipher single event, encrypt/decrypt aes-256-cbc, 16b RANDOM IV, 32b key, b64 encode validate initial cleartext message

That is a typical Java/JRuby openssl cipher error when you don't have high security crypto policy files installed on a JVM

https://github.com/jruby/jruby/wiki/UnlimitedStrengthCrypto

Installing the files in your JRE install should solve the issue. OR if you can't, change your config to 128bit aes such as the below.

    filter {

        cipher {
            algorithm => "aes-128-cbc"
            cipher_padding => 1
            iv_random_length => 16
            key => "1234567890123456"
            key_size => 16
            mode => "encrypt"
            source => "message"
            target => "message_crypted"
            base64 => true
            max_cipher_reuse => 1
        }

        cipher {
            algorithm => "aes-128-cbc"
            cipher_padding => 1
            iv_random_length => 16
            key => "1234567890123456"
            key_size => 16
            mode => "decrypt"
            source => "message_crypted"
            target => "message_decrypted"
            base64 => true
            max_cipher_reuse => 1
        }
    }

Let me know how you guys want to proceed. Put a note about this in the docs and change the test to the lower-common-denominator (aes-128)?

+1 on both your suggestions. after that I'll merge into master and release

That's what I'd suggest. The tests certainly should not use higher than
standard length keys, for simplicity sake. A note in the docs would
undoubtedly save someone time in the long run.

On Wed, Jan 27, 2016 at 10:10 AM, bitsofinfo [email protected]
wrote:

That is a typical Java/JRuby openssl cipher error when you don't have high
security crypto policy files installed on a JVM

https://github.com/jruby/jruby/wiki/UnlimitedStrengthCrypto

Installing the files in your JRE install should solve the issue. OR if you
can't, change your config to 128bit aes such as the below.

filter {

    cipher {
        algorithm => "aes-128-cbc"
        cipher_padding => 1
        iv_random_length => 16
        key => "1234567890123456"
        key_size => 16
        mode => "encrypt"
        source => "message"
        target => "message_crypted"
        base64 => true
        max_cipher_reuse => 1
    }

    cipher {
        algorithm => "aes-128-cbc"
        cipher_padding => 1
        iv_random_length => 16
        key => "1234567890123456"
        key_size => 16
        mode => "decrypt"
        source => "message_crypted"
        target => "message_decrypted"
        base64 => true
        max_cipher_reuse => 1
    }
}

Let me know how you guys want to proceed. Put a note about this in the
docs and change the test to the lower-common-denominator (aes-128)?

—
Reply to this email directly or view it on GitHub
#3 (comment)
.

Added a note and changed tests to 128bits, @jsvd please try running again, should work for you now if you don't have the JCE unlimited strength policy files installed for your JRE

merged in 01311b3 to master

version 2.0.3 has been published https://rubygems.org/gems/logstash-filter-cipher/versions/2.0.3

Thank you for the patience @bitsofinfo.