-
Notifications
You must be signed in to change notification settings - Fork 717
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add TLS and basic authentication integration to Logstash API server #7408
Conversation
ecd1518
to
4395ddf
Compare
…support_https_api # Conflicts: # config/samples/logstash/logstash_stackmonitor.yaml
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I left a comment on testing.
Also - what is the effort to make the api.*
settings support setting with secrets and ENV variables, rather than putting the password in clear in the Config
object?
Running through the example and seeing:
Spec:
Config:
api.auth.basic.password: i_am_rich
api.auth.basic.username: batman
api.auth.type: basic
when running k describe logstashes.logstash.k8s.elastic.co logstash-sample
is something we should fix before we GA this feature
Name: LogstashAPIServiceName, | ||
TLS: commonv1.TLSOptions{ | ||
SelfSignedCertificate: &commonv1.SelfSignedCertificate{ | ||
Disabled: false, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This test passes regardless of whether the value here has the TLS block, or whether selfSignedCertificate.Disabled
value is true
or false
, as only the useTLS
flag is relevant here.
I think we are missing a layer of testing, of how the useTLS
setting is derived from the TLSOptions
@robbavey Thanks for testing this PR. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM on what is up so far.
…ure settings) and env - integrate the resolved value to stack monitoring, readinessProbe - store the API keystore password in config secret for initConfigContainer
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Still working through it, but adding my thoughts as I go
@@ -699,6 +700,97 @@ spec: | |||
The name of the container in the Pod template must be `logstash`. | |||
|
|||
|
|||
[id="{p}-logstash-http-config"] | |||
== HTTP Configuration |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we need to qualify that this is related to the Monitoring API/API service only, and tie it back to the use of stack monitoring, maybe something like "Securing Monitoring Endpoint" with a discussion that we use HTTPS by default, but this can be changed following the instructions here
docs/orchestrating-elastic-stack-applications/logstash.asciidoc
Outdated
Show resolved
Hide resolved
docs/orchestrating-elastic-stack-applications/logstash.asciidoc
Outdated
Show resolved
Hide resolved
pkg/controller/logstash/config.go
Outdated
|
||
logstashv1alpha1 "github.com/elastic/cloud-on-k8s/v2/pkg/apis/logstash/v1alpha1" | ||
"github.com/elastic/cloud-on-k8s/v2/pkg/apis/logstash/v1alpha1" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there a reason we lost the logstash
disambiguation here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
revert it.
elasticsearchRef: | ||
name: "elasticsearch-sample" | ||
services: | ||
- name: api |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we should add a note that the service
name has to be api
pkg/controller/logstash/config.go
Outdated
// and Keystore from SecureSettings. If the same key defined in both places, keystore takes the precedence. | ||
func getKeystoreEnvKeyValue(params Params) (map[string]string, error) { | ||
data := make(map[string]string) | ||
c := getLogstashContainer(params.Logstash.Spec.PodTemplate.Spec.Containers) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Return value from getLogstashContainer
needs to be checked, and we should add a warning if no container is found -
this can be nil
if no explicit podTemplate.spec.containers
block is set, which will cause the operator to crash hard when subsequently attempting to read env
values from it
pkg/controller/logstash/config.go
Outdated
// VAR is the variable name that expect to be defined in Keystore or Env | ||
// | ||
// The variable name can consist of digit, underscores and letters | ||
// The default value is optional, ${VAR:} and ${VAR} are valid. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wonder if we need to support the default
case, and if we can just stick with the standard ${VAR}
? If we do support the default
case, we should log that we are using the default
As far as supporting default values, I think that allowing defaults will only work when there are already ENV variables set in the podTemplate
container definition as it stands, so the use case for supporting defaults is potentially narrow, and potentially a source of confusion.
Also, I'm not sure if allowing defaults is typical in Kubernetes - I don't believe that envFrom
supports it's use
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Supporting default value is mainly for keeping consistent of the feature that Logstash provide. The default value is used when the ${VAR} is missing in env and keystore. It allows Logstash starts without raising error.
apiVersion: logstash.k8s.elastic.co/v1alpha1
kind: Logstash
metadata:
name: logstash-sample
spec:
count: 1
version: 8.11.1
config:
api.auth.type: basic
api.auth.basic.username: "${API_USERNAME:batman}"
api.auth.basic.password: "${API_PASSWORD:i_am_rich}"
Supporting default is not a thing for Kubernetes, but for Stack Monitoring and ReadinessProbe query monitoring endpoint.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, that makes sense, but I'd still like to see a log if we do revert to default
values (not logging the default values...)
But I'm still on the fence whether we need the complexity of adding support for default values for ENV variables for this feature only, as I suspect adding default values for usernames and passwords is not going to be a massively used feature, and may be something we can document our way around.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Agree having default value for username password is a small use case and not a good practice. This makes me think why Logstash supports this feature in logstash.yml
at the first place. For pipelines config, this is a handy features for business logic. However, for logstash config, all settings are important, so, if I expect a ${VAR} and it is missing, I only want to see error and stop Logstash.
Probably, the default value is just a fallback setup to disable basic authentication and SSL in testing phase.
api.ssl.enabled: "${SSL_ENABLED:false}"
api.auth.basic.username: "${API_USERNAME:}"
api.auth.basic.password: "${API_PASSWORD:}"
I think it is acceptable to remove the feature. Do you see any downside to keep it? I am open to remove the feature.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, for business logic in pipelines, it's very useful, but it feels a little less so in this case, and silently falling back to defaults for passwords is something I worry about.
How much effort is it to remove? And how much would it simplify?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's remove it then. I will need to update doc, test cases and the config.go
pkg/controller/logstash/config.go
Outdated
// and Keystore from SecureSettings. If the same key defined in both places, keystore takes the precedence. | ||
func getKeystoreEnvKeyValue(params Params) (map[string]string, error) { | ||
data := make(map[string]string) | ||
c := getLogstashContainer(params.Logstash.Spec.PodTemplate.Spec.Containers) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The return value from getLogstashContainer
should be checked - the value will be nil
with a CRD definition not containing an explicit container definition in the podTemplate
spec, which will cause a crash in subsequent checks of the Env
and EnvFrom
, etc below.
<1> Store the username and password in a Secret. | ||
<2> Map the username and password to the environment variables of the Pod. | ||
<3> At Logstash startup, `${API_USERNAME}` and `${API_PASSWORD}` are replaced by the value of environment variables. Check links:https://www.elastic.co/guide/en/logstash/current/environment-variables.html[using environment variables] for more details. | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe a reference somewhere to using the logstash keystore?
Co-authored-by: Rob Bavey <rob.bavey@elastic.co>
Co-authored-by: Peter Brachwitz <peter.brachwitz@gmail.com>
@barkbay Are you getting the docker image released by Elastic? openssl command is there since 7.17.0 and I can see it is in your testing version 8.11.1 Running
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@barkbay Are you getting the docker image released by Elastic? openssl command is there since 7.17.0 and I can see it is in your testing version 8.11.1
Not in the UBI images until version 8.12.0
IIUC:
❌ docker.elastic.co/logstash/logstash-ubi8:8.11.1
docker run --rm --entrypoint openssl docker.elastic.co/logstash/logstash-ubi8:8.11.1 version
docker: Error response from daemon: failed to create task for container: failed to create shim task: OCI runtime create failed: runc create failed: unable to start container process: exec: "openssl": executable file not found in $PATH: unknown.
✅ docker.elastic.co/logstash/logstash:8.11.1
docker run --rm --entrypoint openssl docker.elastic.co/logstash/logstash:8.11.1 version
OpenSSL 1.1.1f 31 Mar 2020
✅ docker.elastic.co/logstash/logstash-ubi:8.12.0
docker run --rm --entrypoint openssl docker.elastic.co/logstash/logstash-ubi:8.12.0 version
OpenSSL 1.1.1k FIPS 25 Mar 2021
Which means that this feature is likely to break Logstash deployments for users on OpenShift until 8.12.0
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, we probably need a big note in the release notes that we are restricting the version to 8.12 especially for early adopters who are already running Logstash in earlier versions.
if b.MutatedFrom != nil { | ||
builderCopy.MutatedFrom = b.MutatedFrom.DeepCopy() | ||
} | ||
builderCopy.GlobalCA = b.GlobalCA |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You are not copying the APIServer
struct?
if version.MustParse(test.Ctx().ElasticStackVersion).LT(logstashv1alpha1.MinStackMonVersion) { | ||
t.SkipNow() | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this is enforce by the log stash builder already
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: I noticed this error message while running TestLogstashResolvingDollarVariableInStackMonitoring
:
k logs -n e2e-mercury pod/test-ls-mon-dollar-5hq9-ls-0 elastic-internal-init-keystore
+ keystore_initialized_flag=/usr/share/logstash/config/elastic-internal-init-keystore.ok
+ [[ -f /usr/share/logstash/config/elastic-internal-init-keystore.ok ]]
+ echo 'Initializing keystore.'
Initializing keystore.
+ echo y
+ /usr/share/logstash/bin/logstash-keystore create
Using bundled JDK: /usr/share/logstash/jdk
/usr/share/logstash/vendor/bundle/jruby/3.1.0/gems/concurrent-ruby-1.1.9/lib/concurrent-ruby/concurrent/executor/java_thread_pool_executor.rb:13: warning: method redefined; discarding old to_int
/usr/share/logstash/vendor/bundle/jruby/3.1.0/gems/concurrent-ruby-1.1.9/lib/concurrent-ruby/concurrent/executor/java_thread_pool_executor.rb:13: warning: method redefined; discarding old to_f
ERROR: Failed to load settings file from "path.settings". Aborting... path.setting=/usr/share/logstash/config, exception=LogStash::ConfigurationError, message=>Cannot evaluate `${API_PASSWORD}`. Replacement variable `API_PASSWORD` is not defined in a Logstash secret store or as an Environment entry and there is no default value given.
Sending Logstash logs to /usr/share/logstash/logs which is now configured via log4j2.properties
WARNING: The keystore password is not set. Please set the environment variable `LOGSTASH_KEYSTORE_PASS`. Failure to do so will result in reduced security. Continue without password protection on the keystore? [y/N] [2024-01-22T08:27:35,934][INFO ][org.logstash.secret.store.backend.JavaKeyStore] Created Logstash keystore at /usr/share/logstash/config/logstash.keystore
Created Logstash keystore at /usr/share/logstash/config/logstash.keystore
...
The e2e test is eventually ✅ , I was wondering if it's relevant.
Co-authored-by: Peter Brachwitz <peter.brachwitz@gmail.com>
Co-authored-by: Peter Brachwitz <peter.brachwitz@gmail.com>
- fix tests for minimum version
@barkbay The error message is potentially an item to be improved in Logstash side. The keystore command notices an unresolved |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
pkg/controller/logstash/config.go
Outdated
|
||
// getUnresolvedVars matches pattern ${VAR} against the configuration value in logstash.yml, such as api.auth.basic.username: ${API_USERNAME} | ||
// and gives a map of string and string pointer, for example: {"API_USERNAME" : &config.Username} | ||
// The keys in the map represent variable names that require further resolution, retrieving the values from either the Keystore or Environment variables. | ||
// The variable name can consist of digit, underscores and letters. | ||
func getUnresolvedVars(config *configs.APIServer) map[string]*string { | ||
data := make(map[string]*string) | ||
|
||
pattern := `^\${([a-zA-Z0-9_]+)}$` | ||
regex := regexp.MustCompile(pattern) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
// getUnresolvedVars matches pattern ${VAR} against the configuration value in logstash.yml, such as api.auth.basic.username: ${API_USERNAME} | |
// and gives a map of string and string pointer, for example: {"API_USERNAME" : &config.Username} | |
// The keys in the map represent variable names that require further resolution, retrieving the values from either the Keystore or Environment variables. | |
// The variable name can consist of digit, underscores and letters. | |
func getUnresolvedVars(config *configs.APIServer) map[string]*string { | |
data := make(map[string]*string) | |
pattern := `^\${([a-zA-Z0-9_]+)}$` | |
regex := regexp.MustCompile(pattern) | |
const ( | |
varPattern = `^\${([a-zA-Z0-9_]+)}$` | |
) | |
var ( | |
varRegex = regexp.MustCompile(varPattern) | |
) | |
// getUnresolvedVars matches pattern ${VAR} against the configuration value in logstash.yml, such as api.auth.basic.username: ${API_USERNAME} | |
// and gives a map of string and string pointer, for example: {"API_USERNAME" : &config.Username} | |
// The keys in the map represent variable names that require further resolution, retrieving the values from either the Keystore or Environment variables. | |
// The variable name can consist of digit, underscores and letters. | |
func getUnresolvedVars(config *configs.APIServer) map[string]*string { | |
data := make(map[string]*string) |
|
||
for _, envFrom := range c.EnvFrom { | ||
// from ConfigMap | ||
if envFrom.ConfigMapRef != nil { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
IIUC the referenced ConfigMap
or Secret
are not watched? I was wondering if we should do something if they are updated.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes they are not watched. I think it is okay to let user manages the pod restart to get latest value
} | ||
|
||
func (server APIServer) UseTLS() bool { | ||
switch server.SSLEnabled { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: we could also use https://pkg.go.dev/strconv#ParseBool
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
empty string need to convert to true, but ParseBool() gives false
Co-authored-by: Michael Morello <michael.morello@gmail.com>
…support_https_api # Conflicts: # test/e2e/test/logstash/http_client.go
@pebrc This is ready to merge. Thank you! |
This PR adds TLS/ HTTPS and basic authentication integration to Logstash API server. The minimum support version changes from
8.6.0
to8.12.0
.Sample logstash.yml
HTTPS is on by default meaning
api.ssl.enabled
,api.ssl.keystore.path
andapi.ssl.keystore.password
is set in configlogstash.yml
. The API server (puma jruby) only supports HTTPS with p12 keystore and java keystore. Therefore, InitContainer needs to covert CA and TLS certs to the format puma accepts. Ifapi.ssl.enabled
set to true and the API Service is set to disable TLStls.selfSignedCertificate.disabled
, reconcile config fails. If API Service is set to disable andapi.ssl.enabled
is unset, server will disable TLS.Logstash resolves
${VAR}
from ENV and Keystore. When the same key is declared in both places, keystore takes the precedence. As Logstash allows setting HTTP basic authentication withapi.auth.type
,api.auth.basic.username
andapi.auth.basic.password
inlogstash.yml
, this PR has integrated ReadinessProbe and Stack Monitoring by passing the resolved value of username password. The value of the variable comes from the following sources in the order of priority: Env, Env from ConfigMap, Env from Secret, Keystore from Secure Settings . The later sources take precedence.Sample config
The sample config creates following resources
In the past, Secret/logstash-sample-ls-config only stored the
logstash.yml
content. Now it stores the resolved value of api.ssl.keystore.password under the Secret keyAPI_KEYSTORE_PASS
for not exposing the password in plain text in initConfigContainere2e test
fix: #6971, https://github.com/elastic/ingest-dev/issues/1591