Skip to content
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

Support env variable in condition #13608

Merged

Conversation

kaisecheng
Copy link
Contributor

@kaisecheng kaisecheng commented Jan 12, 2022

Release notes

Support environment variable in condition.

Motivation

Two substitutions in ruby side cause inconsistent precedence when resolving the variable.
In the past, ${VAR:default_value} is resolved in the following precedence: secret store -> environment variable -> default value.
After two substitutions, (first substitute) environment variable -> default value -> (second substitute) secret store
The inconsistency could confuse users.

To truly support if with value in secret store, this commit implement substitution in java side

What does this PR do?

This PR substitutes ${VAR} in Expression, except RegexValueExpression, with the value in secret store, env.
The substitution happens after syntax parsing and before graph execution.

Why is it important/What is the impact to the user?

This PR allows users to use environment variable in condition, eg. if [app] == "${VAR}"

Checklist

  • My code follows the style guidelines of this project
  • I have commented my code, particularly in hard-to-understand areas
  • I have made corresponding changes to the documentation
  • I have made corresponding change to the default configuration files (and/or docker env variables)
  • I have added tests that prove my fix is effective or that my feature works

Author's Checklist

  • checked expressions. (), !(), >, >=, <, <=, ==, !=, in, not in, =~, !~, and, or, nand, xor
  • regex /${VAR}/ is not expected to do substitution
  • checked multiple and. () and () and ()
  • secret value deso not show in plain text when the pipeline config has syntax error
  • undefined env variable show error msg when Logstash start

Known Limitation

Substitution only evaluates ${VAR} to string.
if [app] == "${VAR}" is a valid statement. VAR is resolved as a string.
if [app] == 1 is a valid statement, but if [app] == ${VAR} is a wrong syntax. VAR cannot be evaluated to a number, hence cannot do number comparison.
This is the same behavior as of now. The following count is a string and cannot be a number

generator {
  count => "${count}"
}

How to test this PR locally

input {
	generator {
		lines => ['{"app": "foobar"}']
	}
}
filter {
	json {
		source => "message"
	}
	if [app] == "${VAR}" {
		mutate { add_tag => ["a_tag"] }
	}
}
output {
	stdout { codec => rubydebug }
}

run logstash with above config should see tag
$ VAR=foobar bin/logstash

Related issues

Fixed: #5115

Use cases

Screenshots

Logs

…ted BooleanExpression cache. The cache is referenced after compiling to graph.
Copy link
Member

@jsvd jsvd left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Overall LGTM, with one concern that is out of scope for this PR: it is quite easy to use a secret from they keystore in a place that is not a password property. In this case it's likely that the value will be logged raw.

On one hand we want users to make their own decisions, but we should guide them towards best practices. We need to track this concern and think of ways to improve on it, maybe by having the cve tell us where the value came from (env or keystore) and log a warning if the element isn't a password setting. Another alternative is to only allow retrieving from the keystore on password setting elements (more intrusive).

import org.logstash.config.ir.graph.QueueVertex;
import org.logstash.config.ir.graph.Vertex;
import org.logstash.config.ir.graph.SeparatorVertex;
import org.logstash.config.ir.graph.*;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We tend do avoid wildcard imports, and you can make intellij not do them automatically, see: https://sergiodelamo.com/blog/intellij-idea-disable-wildcard-imports.html

@kaisecheng kaisecheng merged commit 057c24a into elastic:main Jan 25, 2022
kaisecheng added a commit to kaisecheng/logstash that referenced this pull request Jan 25, 2022
This PR substitutes ${VAR} in Expression, except RegexValueExpression, with the value in secret store, env.
The substitution happens after syntax parsing and before graph execution.

Fixed: elastic#5115
kaisecheng added a commit to kaisecheng/logstash that referenced this pull request Jan 25, 2022
This PR substitutes ${VAR} in Expression, except RegexValueExpression, with the value in secret store, env.
The substitution happens after syntax parsing and before graph execution.

Fixed: elastic#5115
yaauie pushed a commit that referenced this pull request Jan 26, 2022
This PR substitutes ${VAR} in Expression, except RegexValueExpression, with the value in secret store, env.
The substitution happens after syntax parsing and before graph execution.

Fixed: #5115
yaauie pushed a commit that referenced this pull request Jan 26, 2022
This PR substitutes ${VAR} in Expression, except RegexValueExpression, with the value in secret store, env.
The substitution happens after syntax parsing and before graph execution.

Fixed: #5115
@kaisecheng
Copy link
Contributor Author

@jsvd Regarding showing log with secret in non password property, I am wondering in what situation we show such log.
I can think of enabling debug log, the pipeline with password is printed in plaintext. For syntax error case, it does not log substitution. Do you have other cases in your mind that can trigger log with secret?

@jsvd
Copy link
Member

jsvd commented Jan 26, 2022

I can think of enabling debug log

This is the major one. it means that any value in the secret store is dumped raw to disk when someone starts Logstash in debug mode unless it's in a password setting. Historically we've tried to gate this exposure behind the extra "config.debug" flag:

--config.debug
Show the fully compiled configuration as a debug log message (you must also have --log.level=debug enabled). WARNING: The log message will include any password options passed to plugin configs as plaintext, and may result in plaintext passwords appearing in your logs!

Again, this should not be an issue and users should understand that if it's in keystore, it should be put into a password setting that mascarades itself when logged, but we can also expect that someone will do something like:

input { http { } }
filter {
  if [header][auth] != "${secret}" {
    drop {}
  }
}

Then at some point run the pipeline in debug mode.

nickumia-reisys added a commit to GSA/datagov-logstack that referenced this pull request May 1, 2022
A new feature simplifies the logstash configuration file: elastic/logstash#13608
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Support Environment Variable in conditionals
3 participants