falco
falco copied to clipboard
falcosecurity
feat: support parsing of system environment variables in yaml
What type of PR is this?
/kind feature
Any specific area of the project related to this PR?
/area engine /area tests
What this PR does / why we need it:
In order to allow the user to supply environment variables in standard
ways performed in other applications the get_scalar function has been
extended to support defining an environment variable as either the
format $FOO or the format ${FOO}. As this handles some additional
complexity a unit test has been added to cover this new functionality
Which issue(s) this PR fixes: Fixes #2561
Special notes for your reviewer:
Does this PR introduce a user-facing change?:
feat: support parsing of system environment variables in yaml
Welcome @therealdwright! It looks like this is your first PR to falcosecurity/falco 🎉
This is interesting! Thanks a lot. I'm gonna set this to the current release milestone just to not lose track of it, however keep in mind that we're very close to a Falco release and getting this into it will require a decision among the Falco maintainers and the release managers.
/milestone 0.35.0
cc @falcosecurity/falco-maintainers
LGTM label has been added.
- How can I escape from it if I wanted the literal
$SOMETHINGas a string (instead of the env var value)? 🤔
In most other applications, you would quote the string e.g. "$SOMETHING" . However, This has not something I have come across in practice. This implementation looks for a { at position 1, and a } at position n - 1, so it only handles unquoted environment strings, consistent with other OSS implementations (e.g. filebeat).
- How this feature interact with the program_output which is a shell code? 🤔
The above response covers this somewhat, but I also tested the original behaviour and ran regression tests while developing the change. Does the regression suite cover such cases?
In terms of timeline, I'll leave it to the maintainers, I selfishly want this sooner rather than later as not being able to define environment variables leads to one of two consequences for my organisation.
- We have to encrypt the entire configuration file with something like sops if we want to use the current implementation as is.
- We must write some custom tooling to extend the upstream Falco image to iterate through the YAML and eval all environment variables.
Neither of these are the direction we want to take, but if we have to, so be it.
Edit: please let me know if you want additional unit testing to cover such cases, I would be happy to do so.
- How can I escape from it if I wanted the literal
$SOMETHINGas a string (instead of the env var value)? thinkingIn most other applications, you would quote the string e.g.
"$SOMETHING". However, This has not something I have come across in practice. This implementation looks for a{at position 1, and a}at position n - 1, so it only handles unquoted environment strings, consistent with other OSS implementations (e.g. filebeat).
- How this feature interact with the program_output which is a shell code? thinking
The above response covers this somewhat, but I also tested the original behaviour and ran regression tests while developing the change. Does the regression suite cover such cases?
AFAIK, no.
The filebeat example is interesting. I have never used it so, but their docs report:
If you need to use a special character in your configuration file, use $ to escape the expansion. For example, you can escape ${ or } with $${ or $}.
Anyway, I'm not saying this is necessarily an issue - I just don't know yet and I couldn't find enough time to iterate on that :sweat_smile:
In terms of timeline, I'll leave it to the maintainers, I selfishly want this sooner rather than later as not being able to define environment variables leads to one of two consequences for my organisation.
:+1: I'm open to any decision, just the timeline is unfortunate. Let's see other maintainers' opinions.
- We have to encrypt the entire configuration file with something like sops if we want to use the current implementation as is.
- We must write some custom tooling to extend the upstream Falco image to iterate through the YAML and eval all environment variables.
Neither of these are the direction we want to take, but if we have to, so be it.
Out of curiosity: have you ever tried using --option as a workaround? For example:
falco ... --option someopt=$SOMEVAR
N.B. I'm not saying this is a clean solution.
Let me know if you would like this PR extended to handle the special case scenario similarly to filebeat. I'm happy to update the PR.
Out of curiosity: have you ever tried using --option as a workaround? For example:
I wasn't aware this was an option; I'll take a look tomorrow and give it a try.
Unfortunately, moving forward with http_output configured as an option did the job of getting the config in but resulted in a new bug which has been raised as #2579
I'd move this to 0.36.0 since we have not reach an agreement for this to get merged in time for 0.35. I am sorry! We will review it asap right after 0.36.0
/milestone 0.36.0
@FedeDP - shall I abandon this PR? It has been some time now and there's not really been any engagement.
shall I abandon this PR? It has been some time now and there's not really been any engagement.
I think it is good indeed! Let me ask for a review :) Sorry for the long time no see, we were all very busy with cncf graduation related stuff!
/cc @leogr @jasondellaluce
- Using just one format for env variables, like
${VAR}and not also$VAR. No strong opinion but IMO having just one way to use env var would be easier for end users
No strong opinion.
- With these changes if a user specifies
${VAR}in the config it will be resolved to an env variable but maybe this is not what the user want, so I would propose a possible escaping method like adding an extra$, not so smart but it should be effective and easy to implement:Example
- Setting
VAR=hello- if in the config we use
${VAR}we will resolve it to "hello", while if we use$${VAR}we will keep only${VAR}without resolving it
I'm totally on board with this.
However, since this has been waiting for a long time, I understand there are compelling use cases, I'm also ok let merging this PR as-is (maybe just removing the $VAR support) and improving it in a followup PR.
However, since this has been waiting for a long time, I understand there are compelling use cases, I'm also ok let merging this PR as-is (maybe just removing the $VAR support) and improving it in a followup PR.
I'd ask the PR author whether he is willing to quickly fix this up @therealdwright , otherwise i am on board with merging this as is, and then rework it later.
if in the config we use
${VAR}we will resolve it to "hello", while if we use$${VAR}we will keep only${VAR}without resolving it
This will take a bit of extra care to handle, for now I have kept the PR to only handle the ${VAR} case to keep things simple and if there is a strong desire to add variable esscaping please let me know and I will push up a follow-up. I think this is a vary rare edge case that we are unlikely to see in the real world and implementing will require some thought and user-facing documentation.
OK @Andreagit97 @leogr and @FedeDP
This has taken me a bit to get around as I have not thought about or touched this codebase for three months, and each piece of feedback in isolation was reasonable, but there were some conflicts to resolve to appease it all. I think I have struck a good balance here.
- Only one format of an environment variable is now supported, being
${ENV_VAR} - YAML values following a
$ENV_VARformat will be processed as a regular string - YAML Values following a
$${ENV_VAR}format will be processed as a regular string with the first $ stripped - YAML values following a
$$ENV_VARformat will be processed as a regular string
I think I have adequately covered the various edge cases in the unit test, as I hope you can appreciate there are a lot of edge cases to check for here. Please re-review and let me know what you think. I would appreciate it if the team could come back to me soon as it's very hard to step away from this project and C++ for so long and be productive.
If you would like me to write some user-facing docs, LMK, I'd be happy to do so.
- Only one format of an environment variable is now supported, being
${ENV_VAR}- YAML values following a
$ENV_VARformat will be processed as a regular string- YAML Values following a
$${ENV_VAR}format will be processed as a regular string with the first $ stripped- YAML values following a
$$ENV_VARformat will be processed as a regular string
Agree with the design, thanks!
LGTM! Agree with @Andreagit97 comments, then we are good to go! Thanks for the effort @therealdwright !
+1 for me too! Thank you!
LGTM label has been added.
For posterity, PR description and commit message body have been updated to reflect the new state of the change.
[APPROVALNOTIFIER] This PR is APPROVED
This pull-request has been approved by: Andreagit97, FedeDP, therealdwright
The full list of commands accepted by this bot can be found here.
The pull request process is described here
- ~~OWNERS~~ [Andreagit97,FedeDP]
Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment