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

use envvars as default value for sensitive argments. #21

Merged

Conversation

jspaleta
Copy link
Contributor

@jspaleta jspaleta commented Jan 4, 2019

Treat influx addr, username and password as sensitive credentials.

It's preferable to read sensitive information from envvars.
commandline arguments can be read from the process table for ex: ps -aux

Solution in this PR:

  1. Use os.Getenv() to retrieve default values from env vars when constructing command flags.
  2. Do not mark these as required flags, if not set and env var defaults are used.
  3. Manual test for empty string values and error out accordingly.

Copy link

@nikkictl nikkictl left a comment

Choose a reason for hiding this comment

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

I love how you implemented this with backwards compatibility! Since the help usage changed, we'll need to update the readme with the new flag descriptions. If we expect and encourage users to store the sensitive data as environment variables, we should also state that in the readme, and perhaps provide short instruction on how to do so. Lastly, we'll need a changelog entry in the "Unreleased" section.

This is going to be a great security win. Code-wise, this LGTM 👍

main.go Outdated Show resolved Hide resolved
@jspaleta
Copy link
Contributor Author

jspaleta commented Jan 5, 2019

requested changes made.
I have locally built version running now using envvars

sensuctl handler info influxdb-particle
=== influxdb-particle
Name:                  influxdb-particle
Type:                  pipe
Timeout:               30
Filters:               
Mutator:               
Execute:               RUN:  /usr/local/bin/sensu-influxdb-handler -d 'particle'
Environment Variables: INFLUXDB_ADDR=http://localhost:8086, INFLUXDB_USER=nunya, INFLUXDB_PASS=business

Copy link

@nikkictl nikkictl left a comment

Choose a reason for hiding this comment

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

💯

@nikkictl nikkictl merged commit 45d52de into sensu:master Jan 8, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants