[receiver/jmxreceiver] Limit parameters for JMX receiver to reduce security risk #9721
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
added 7 commits
May 2, 2022 21:55
djaglowski
reviewed
May 4, 2022
djaglowski
approved these changes
May 5, 2022
codeboten
reviewed
May 5, 2022
| ResourceAttributes map[string]string `mapstructure:"resource_attributes"` | ||
| // Log level used by the JMX metric gatherer. Should be one of: | ||
| // `"trace"`, `"debug"`, `"info"`, `"warn"`, `"error"`, `"off"` | ||
| LogLevel string `mapstructure:"log_level"` |
There was a problem hiding this comment.
Any thoughts on whether this should re-use the log level set in the collector's service.logging setting instead of adding its own?
There was a problem hiding this comment.
I'm open to it. The only drawbacks I see are that there's no Trace equivalent in zap.Level and several levels (fatal, panic, dpanic, error) would probably all map to error (or possibly panic/dpanic to "off").
There was a problem hiding this comment.
@codeboten implemented it so if no user config is provided, it attempts to translate from zap logger level, otherwise it uses the user provided config. Let me know what you think!
Co-authored-by: Alex Boten <alex@boten.ca>
djaglowski
approved these changes
May 13, 2022
|
@codeboten, mind taking another look? |
codeboten
approved these changes
May 18, 2022
jamesmoessis
pushed a commit
to jamesmoessis/opentelemetry-collector-contrib
that referenced
this pull request
May 20, 2022
…curity risk (open-telemetry#9721) * Remove options considered too permissive for security in jmx receiver * Use config file for communicating to JMX metrics gatherer * Fix small issues with jmx receiver * Ensure property files are generated correctly with multiline input * Update changelog * minor changes to receiver to support more config in properties file * Update properties file generation * Address PR feedback and address environment variable attack surface * Update receiver/jmxreceiver/config.go Co-authored-by: Alex Boten <aboten@lightstep.com>
kentquirk
pushed a commit
to McSick/opentelemetry-collector-contrib
that referenced
this pull request
Jun 14, 2022
…curity risk (open-telemetry#9721) * Remove options considered too permissive for security in jmx receiver * Use config file for communicating to JMX metrics gatherer * Fix small issues with jmx receiver * Ensure property files are generated correctly with multiline input * Update changelog * minor changes to receiver to support more config in properties file * Update properties file generation * Address PR feedback and address environment variable attack surface * Update receiver/jmxreceiver/config.go Co-authored-by: Alex Boten <aboten@lightstep.com>
15 tasks
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Description: Update the JMX Receiver as a part of the effort to reduce its attack surface. This PR removes the generic
propertiesparameter, removes the ability to use a user-defined groovy script, and communicates with the subprocess via a properties file rather than command line parameters.Link to tracking Issue: #9686 #9685
Testing: Manually validated in addition to updates to the unit tests. Integration tests for JMX receiver are currently flaky and require improvement.