Support TLS by socket/server plugin helper #1423
Conversation
tagomoris
added
feature request
|
@repeatedly could you review this change? |
|
Okay. I will review it later. |
| if @tls_cert_path && !@tls_cert_path.empty? | ||
| @tls_cert_path.each do |path| | ||
| raise Fluent::ConfigError, "specified cert path does not exist:#{path}" unless File.exist?(path) | ||
| raise Fluent::ConfigError, "specified cert path is not readable:#{path}" unless File.exist?(path) |
There was a problem hiding this comment.
Oops. It's just mistake.
| host_is_hostname = !(IPAddr.new(@host) rescue false) | ||
| @hostname = case | ||
| when host_is_hostname then @host | ||
| when @name then @name |
There was a problem hiding this comment.
using @name is correct way for certificate verification?
<server>name xxx</server> is not limitation for now.
There was a problem hiding this comment.
Yes, I changed the meaning of name in <server> section.
But in past versions, this name is used to indicate the role (or other human readable thing) of this server node in logging.
The "human readable role" is just FQDN server name signed by server certificate, isn't it?
| cert_store.add_file(cert_path) | ||
| end | ||
| else | ||
| cert_path = cert_paths |
There was a problem hiding this comment.
Why re-assing to cert_path? just for readability?
| cert_store.set_default_paths | ||
| end | ||
| rescue OpenSSL::X509::StoreError | ||
| log.warn "failed to load system default certificate store", error: e |
There was a problem hiding this comment.
Is warn ok?
This failure is not critical for actual connection?
There was a problem hiding this comment.
It depends on many things including env variables to specify default cert store path configured by end users, and not to make connections failure directly (because many other parameters can control it - e.g., self signed certs, manually specified cert path and others).
So, I left it as warned, not critical.
| @@ -76,6 +81,19 @@ class ConnectionClosedError < Error; end | |||
| desc 'Compress buffered data.' | |||
| config_param :compress, :enum, list: [:text, :gzip], default: :text | |||
|
|
|||
| desc 'The default version of TLS transport.' | |||
| config_param :tls_version, :enum, list: Fluent::PluginHelper::Socket::TLS_SUPPORTED_VERSIONS, default: Fluent::PluginHelper::Socket::TLS_DEFAULT_VERSION | |||
There was a problem hiding this comment.
Using tls_ prefix instead of <tls> is for avoiding confustion with <transport tls>?
There was a problem hiding this comment.
As you mentioned, these parameters are named in different way from servers' <transport> section.
I think that almost users just specify one or (at most) two parameters in these tls_ prefixed parameters (tls_cert_path, and sometimes tls_allow_self_signed_cert). Many users don't change the default TLS version nor TLS ciphers.
In such cases, tls_cert_path ... is much more simple than <tls> tls_cert_path ... </tls>.
|
Sorry for the late. LGTM |
…onfiguration files
tagomoris
force-pushed
the
support-tls-by-server-plugin-helper
branch
from
3d75306 to
9a011dd
Compare
|
Rebased. I'll merge this after CI green. |
|
Green! |
This change is to add a functionality to support TLS, mainly for in/out forward plugins.
The server plugin helper's TLS support is:
tls_optionshash argument to specify various options about TLS options and certifications in programatic way<transport tls>section of configuration transparently to control them by users directly (not to write any code in plugins)This feature supports these certificate management:
With this feature, v0.14 forward input/output plugins are compatible with 3rd party secure-forward plugin.