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 for passing username, password & priv_password as env vars #1074

Merged
merged 5 commits into from
Feb 19, 2024

Conversation

harshavmb
Copy link
Contributor

Hi,

This PR aims to pass sensitive data such as username, password & priv_password as environment variables.

The exporter binary looks for the environment variable presence, if present it fetches the value & pass the same as configuration to the binary.

I know in the past #459 was closed. Nevertheless, times have changed & new set of apps inject env vars in a safe manner. For instance Azure functions have a clever way of injecting keyvault references as env vars for the apps running in their environment. In this case, there is no need of resolving the template or writing unnecessary entrypoint scripts to circumvent them.

Looking forward to have your feedback on this.

@candlerb
Copy link
Contributor

Now that auths are completely separated out, and snmp_exporter can read multiple config files, I see less of a need for this. For example, in Kubernetes you can create a secret with all the auths you need, and load it in addition to the main snmp.yml.

@harshavmb
Copy link
Contributor Author

Hi @candlerb,

Thanks for your feedback. As I mentioned in the description, we are targeting this deployment on Azure functions where in we can inject credentials directly as env vars.

Many apps support credentials to be loaded via environment variables. Are there any constraints wrt., snmp_exporter with this approach I am not aware of?

Copy link
Member

@SuperQ SuperQ left a comment

Choose a reason for hiding this comment

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

In order to support this, I would like to see two changes.

  • This should be behind a flag, disabled by default, for backwards compatibility. --config.expand-environment-variables.
  • Use https://pkg.go.dev/os#Expand instead of a custom parser.

This would make it better match the Prometheus behavior (prometheus/prometheus#8649).

@SuperQ
Copy link
Member

SuperQ commented Feb 4, 2024

Are you still interested in implementing this? I think it would be very much appreciated.

@harshavmb
Copy link
Contributor Author

Hi @SuperQ,

I am on vacation & will be back next week. I shall rework this #PR as per your review.

I hope it's okay for you.

Thanks,
Harsha

Signed-off-by: Harshavardhan Musanalli <Harshavardhan.Musanalli@amadeus.com>
@harshavmb
Copy link
Contributor Author

Hi @SuperQ ,

I've made added --config.expand-environment-variables config (disabled by default) to resolve env vars & removed custom parser.

Could you please share your feedback?

Copy link
Member

@SuperQ SuperQ left a comment

Choose a reason for hiding this comment

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

Some minor documentation issues. Otherwise looks good.

README.md Outdated Show resolved Hide resolved
README.md Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
harshavmb and others added 3 commits February 19, 2024 09:02
Co-authored-by: Ben Kochie <superq@gmail.com>
Signed-off-by: Harshavardhan Musanalli <harshavmb@gmail.com>
Co-authored-by: Ben Kochie <superq@gmail.com>
Signed-off-by: Harshavardhan Musanalli <harshavmb@gmail.com>
Co-authored-by: Ben Kochie <superq@gmail.com>
Signed-off-by: Harshavardhan Musanalli <harshavmb@gmail.com>
@harshavmb
Copy link
Contributor Author

Hi @SuperQ ,

I made amendments to README.md docs as per the review.

Thanks again for your support here.

@SuperQ SuperQ merged commit 59194f6 into prometheus:main Feb 19, 2024
6 checks passed
@harshavmb harshavmb deleted the envvars branch February 19, 2024 08:59
@RobertBergman
Copy link
Contributor

RobertBergman commented Feb 24, 2024

Hi @SuperQ,

This change broke SNMPv3 by removing the lines that need to go in config.go after 155 and 171 respectively:
usm.AuthenticationPassPhrase = string(c.Password)

and

usm.PrivacyPassphrase = string(c.PrivPassword)

after adding the back in to the code, I tried running with having variables set via environment and in the snmp.yml and both worked. without them I was getting the error:

securityParameter.PrivacyPassphrase is required when a privacy protocol is specified

@SuperQ
Copy link
Member

SuperQ commented Feb 24, 2024

@RobertBergman Oops, I'm surprised this didn't show up in our unit tests. Do you want to open a fix PR?

@RobertBergman
Copy link
Contributor

@SuperQ sorry, I'm not familiar with the process.

RobertBergman added a commit to RobertBergman/snmp_exporter that referenced this pull request Feb 24, 2024
…o go in config.go after 155 and 171 respectively:

usm.AuthenticationPassphrase = string(c.Password)

and

usm.PrivacyPassphrase = string(c.PrivPassword)

after adding the back in to the code, I tried running with having variables set via environment and in the snmp.yml and both worked. without them I was getting the error:

securityParameter.PrivacyPassphrase is required when a privacy protocol is specified
@RobertBergman
Copy link
Contributor

RobertBergman commented Feb 24, 2024 via email

RobertBergman added a commit to RobertBergman/snmp_exporter that referenced this pull request Feb 26, 2024
…o go in config.go after 155 and 171 respectively:

usm.AuthenticationPassphrase = string(c.Password)

and

usm.PrivacyPassphrase = string(c.PrivPassword)

after adding the back in to the code, I tried running with having variables set via environment and in the snmp.yml and both worked. without them I was getting the error:

securityParameter.PrivacyPassphrase is required when a privacy protocol is specified

Signed-off-by: Robert Bergman <rob.bergman@gmail.com>
RobertBergman added a commit to RobertBergman/snmp_exporter that referenced this pull request Feb 26, 2024
…o go in config.go after 155 and 171 respectively:

usm.AuthenticationPassphrase = string(c.Password)

and

usm.PrivacyPassphrase = string(c.PrivPassword)

after adding the back in to the code, I tried running with having variables set via environment and in the snmp.yml and both worked. without them I was getting the error:

securityParameter.PrivacyPassphrase is required when a privacy protocol is specified

Signed-off-by: Robert Bergman <rob.bergman@gmail.com>
SuperQ pushed a commit that referenced this pull request Feb 27, 2024
* Add SNMPInflight metric (#1119)

* Add SNMPInflight metric

---------

Signed-off-by: Kakuya Ando <fservak@gmail.com>
Signed-off-by: Robert Bergman <rob.bergman@gmail.com>

* change #1074 broke SNMPv3 by removing the lines that need to go in config.go after 155 and 171 respectively:
usm.AuthenticationPassphrase = string(c.Password)

and

usm.PrivacyPassphrase = string(c.PrivPassword)

after adding the back in to the code, I tried running with having variables set via environment and in the snmp.yml and both worked. without them I was getting the error:

securityParameter.PrivacyPassphrase is required when a privacy protocol is specified

Signed-off-by: Robert Bergman <rob.bergman@gmail.com>

---------

Signed-off-by: Kakuya Ando <fservak@gmail.com>
Signed-off-by: Robert Bergman <rob.bergman@gmail.com>
Co-authored-by: Kakuya Ando <fservak@gmail.com>
@@ -123,6 +123,8 @@ using SNMP v2 GETBULK.
The `--config.file` parameter can be used multiple times to load more than one file.
It also supports [glob filename matching](https://pkg.go.dev/path/filepath#Glob), e.g. `snmp*.yml`.

The `--config.expand-environment-variables` parameter allows passing environment variables into some fields of the configuration file. The `username`, `password` & `priv_password` fields in the auths section are supported. Defaults to disabled.

Copy link
Contributor

Choose a reason for hiding this comment

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

I wonder if "community" should be allowed too, for consistency? (Although this may interact with community having a default value of "public")

Copy link
Contributor

Choose a reason for hiding this comment

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

I see this and without diving deep think that SNMPv3 doesn't use community.

Copy link
Contributor

Choose a reason for hiding this comment

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

You are correct. I mean for use with v1/v2c, which some people still do use, and requires a "secret" community (albeit sent over the wire in clear text)

@SuperQ SuperQ mentioned this pull request May 10, 2024
SuperQ pushed a commit that referenced this pull request May 11, 2024
* [CHANGE] Improve generator parse error handling #1167
* [ENHANCEMENT] generator: Add generator HELP override #1106
* [ENHANCEMENT] Refactoring of Scrape process, fixing multiple module issues #1111
* [ENHANCEMENT] Skip using an interactive terminal in "make docker-generate". #1113
* [ENHANCEMENT] Add SNMPInflight metric #1119
* [FEATURE] Support for passing username, password & priv_password as env vars #1074
* [FEATURE] Add GoSNMP logger #1157
* [FEATURE] Add a "snmp_context" parameter to the URL #1163
* [BUGFIX] generator: curl failed #1094
* [BUGFIX] Fix SNMPv3 password configuration #1122
* [BUGFIX] generator: Update generator User-Agent #1133
* [BUGFIX] generator: fix mibs directory specification for parse_errors command #1135
* [BUGFIX] generator: remove extra character from dell iDrac-SMIv1 MIB #1141
* [BUGFIX] Fix do not expand envvars for empty config fields #1148

snmp.yml changes:
* Updated Cisco MIBs #1180
* Updated Cyberpower MIBs #1124
* Updated servertech_sentry3 #1090
* Added support for Dell iDrac  #1125
---------

Signed-off-by: Sebastian Schubert <basti@schubert.digital>
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

4 participants