Feat: review and deprecate ssl protocol/cipher settings

[kares]

following up on #146:

• deprecating cipher_suites in favor of ssl_cipher_suites (with better validation of the supported cipher set)
• adding ssl_supported_protocols as a replacement for tls_min_version / tls_max_version

[andsel]

Tested locally, with deprecated config settings and the plugin starts cleanly with only the warnings, it doesn't create any breakage.

I've requested changes to know your opinion on those, if you feel it's worthwhile to simplify.

[andsel]

I think that if @ssl is false then @ssl_cipher_suites_final end in:

• empty list if the user configured cipher_suites.
• SslSimpleBuilder.getDefaultCiphers if the user in any other case.
  I think that if ssl => false it shouldn't set anything related to the ciphers

I know that the method build_ssl_params has a guard:

return nil unless @ssl

Maybe isolating the fragment with an if @sslshould help.

Suggested change

	if @ssl && (original_params.key?('cipher_suites') && original_params.key?('ssl_cipher_suites'))
	raise LogStash::ConfigurationError, "Both `ssl_cipher_suites` and (deprecated) `cipher_suites` were set. Use only `ssl_cipher_suites`."
	elsif original_params.key?('cipher_suites')
	@ssl_cipher_suites_final = @cipher_suites
	else
	@ssl_cipher_suites_final = @ssl_cipher_suites
	end
	if @ssl
	if original_params.key?('cipher_suites') && original_params.key?('ssl_cipher_suites')
	raise LogStash::ConfigurationError, "Both `ssl_cipher_suites` and (deprecated) `cipher_suites` were set. Use only `ssl_cipher_suites`."
	@ssl_cipher_suites_final = original_params.key?('cipher_suites') ?
	@cipher_suites :
	@ssl_cipher_suites
	end

[kares]

ssl_cipher_suites_final won't be used since build_ssl_params isn't called.
the reason why I have it this way is due existing convention above:

    # ...
    elsif original_params.key?("verify_mode")
      @ssl_verify_mode_final = @verify_mode
    else
      @ssl_verify_mode_final = @ssl_verify_mode

[andsel]

Same observation as the one above, protect with an if @ssl could make it more readable.

[kares]

if the attr is not set @ssl_supported_protocols_final = @ssl_supported_protocols is just nil

[kares]

here's a compromise refactoring: db159bb ... wdyt?

[andsel]

The compromise is almost good, I would made it more "assertive", I mean that if an if statement return in the positive branch then simplify the negative branch.

so

if !@ssl
  @logger.warn("SSL Certificate will not be used") if @ssl_certificate
  @logger.warn("SSL Key will not be used") if @ssl_key
  @logger.warn("SSL Java Key Store will not be used") if @keystore
  return # code bellow assumes `ssl => true`
elsif !(ssl_key_configured? || ssl_jks_configured?)
  raise LogStash::ConfigurationError, "Certificate or JKS must be configured"
end

becomes:

if !@ssl
  @logger.warn("SSL Certificate will not be used") if @ssl_certificate
  @logger.warn("SSL Key will not be used") if @ssl_key
  @logger.warn("SSL Java Key Store will not be used") if @keystore
  return
end  

# code below assumes `ssl => true`
if !(ssl_key_configured? || ssl_jks_configured?)
  raise LogStash::ConfigurationError, "Certificate or JKS must be configured"
end

[andsel]

Left just maybe a nitpick at #151 (comment) but I think it makes the code more readable.

Everything else seems ok to me.

[andsel]

LGTM