Fix SSL Java Keystore and Truststore support

[edmocosta]

• Fixed SSL Java KeyStore support (SSL settings aren't working when a keystore is provided #168)
• Added ssl_keystore_type configuration
• Added SSL Java TrustStore configurations (ssl_truststore_type, ssl_truststore_path and ssl_truststore_password)

---

Closes: #168

[jsvd]

I wonder if here we should differentiate between our expectations from a key store being used as a trust store vs one doubling as both key store and trust store.
For a "trust store"-only p12 I'd expect that only certificates live in it, and any keys would be a mistake. on the other hand, when reusing a key store as a trust store anything goes, and should probably be discouraged in production.
I'd suggest checking how our other products do it, but in either case leaving here a comment saying that hasTrustStoreEntry could be made to verify either in a strict or loose modes.

[edmocosta]

I did check how ES was handling it before implementing this logic. It's true that in the ES case, they do throw an exception if the truststore has no trusted certificate. Finally, I ended up adding it here as a warning log to keep it similar to other LS plugins and tackle it later on the 2nd phase of the SSL standardization project (functionality). I can change it if you think that's better.

When ES uses the keystore as truststore, it does not enforce the keystore to have a trusted certificate. It's the last option set when all other fallbacks already failed or are not configured.

[jsvd]

On the final part of the sentence, do we know of an invalid password situation that hits this code? from my test setting a wrong password in the trust store shows a different exception:

[2023-11-27T17:22:05,004][ERROR][logstash.inputs.http     ][main] SSL configuration invalid {:exception=>Java::JavaIo::IOException, :message=>"keystore password was incorrect", :cause=>{:exception=>Java::JavaSecurity::UnrecoverableKeyException, :message=>"failed to decrypt safe contents entry: javax.crypto.BadPaddingException: Given final block not properly padded. Such issues can arise if a bad key is used during decryption."}}
[2023-11-27T17:22:05,073][ERROR][logstash.javapipeline    ][main] Pipeline error {:pipeline_id=>"main", :exception=>#<LogStash::ConfigurationError: keystore password was incorrect>, :backtrace=>["/Users/joaoduarte/elastic/logstash-plugins/logstash-input-http/lib/logstash/inputs/http.rb:426:in `new_ssl_simple_builder'", "/Users/joaoduarte/elastic/logstash-plugins/logstash-input-http/lib/logstash/inputs/http.rb:400:in `build_ssl_params'", "/Users/joaoduarte/elastic/logstash-plugins/logstash-input-http/lib/logstash/inputs/http.rb:394:in `create_http_server'", "/Users/joaoduarte/elastic/logstash-plugins/logstash-input-http/lib/logstash/inputs/http.rb:220:in `register'", "/private/tmp/logstash-8.11.1/logstash-core/lib/logstash/java_pipeline.rb:237:in `block in register_plugins'", "org/jruby/RubyArray.java:1987:in `each'", "/private/tmp/logstash-8.11.1/logstash-core/lib/logstash/java_pipeline.rb:236:in `register_plugins'", "/private/tmp/logstash-8.11.1/logstash-core/lib/logstash/java_pipeline.rb:395:in `start_inputs'", "/private/tmp/logstash-8.11.1/logstash-core/lib/logstash/java_pipeline.rb:320:in `start_workers'", "/private/tmp/logstash-8.11.1/logstash-core/lib/logstash/java_pipeline.rb:194:in `run'", "/private/tmp/logstash-8.11.1/logstash-core/lib/logstash/java_pipeline.rb:146:in `block in start'"], "pipeline.sources"=>["config string"], :thread=>"#<Thread:0x2f3b0895 /private/tmp/logstash-8.11.1/logstash-core/lib/logstash/java_pipeline.rb:134 run>"}

[jsvd]

Overall I'm concerned that we let users get to a state that won't work. if we raised an exception here, now, I don't believe there would be any breaking environments that would be working before. We would have to, however, only raise exception if the client auth is set to required.

[edmocosta]

On the final part of the sentence, do we know of an invalid password situation that hits this code? from my test setting a wrong password in the trust store shows a different exception:

That message is indeed confusing. It's coming from the JDK when it tries to read the truststore file here with an invalid password. I think I can catch this exception and re-throw it with a more meaningful message to the user. WDYT?

Overall I'm concerned that we let users get to a state that won't work. if we raised an exception here, now, I don't believe there would be any breaking environments that would be working before. We would have to, however, only raise exception if the client auth is set to required.

Agreed, this concern also applies to other plugins, that currently have minimum to no validation regarding it. Considering it's a new functionally, I agree with you! It's better to avoid those invalid certificates from the very beginning! I'll change it :)

[jsvd]

That message is indeed confusing. It's coming from the JDK when it tries to read the truststore file here with an invalid password. I think I can catch this exception and re-throw it with a more meaningful message to the user. WDYT?

just to clarify, I'm ok with the current exception that happens when the password is incorrect, but I don't think that the message we introduce in this PR is accurate by mentioning that the password may be incorrect at that point:

logger.warn("The provided Trust Store file does not contains any trusted certificate entry: {}. Please confirm this is the correct certificate and the password is correct", trustStoreFile);

[edmocosta]

Sorry, I misunderstood your message. I've changed it to throw an exception and removed the misleading part of the message. Thanks 👍

[jsvd]

While this is probably what we've done before, there may be a better default where the absence of a trust store would mean that the system certificate store would be used. If the goal here is to not break bwc I'd suggest leaving a TODO comment about changing to defaulting to the system store.

[edmocosta]

I agree that a better default needs to be discussed and defined, and it probably will when we start to standardize the SSL functionality across the plugins. This implementation was based on the current code (always use keystore) and ES's implementation (defaults to the keystore when there's no truststore neither CAs configured).

[jsvd]

In the tests I've done manually everything worked correctly, both with certs directly derived from a root CA and using an intermediate CA, with jks and p12 stores.

There is more to do on ensuring good default behaviours when configuring TLS, but they're out of scope for this PR.

LGTM