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

Standardize the fetching of creds #26

Open
tas50 opened this issue Jul 14, 2015 · 5 comments
Open

Standardize the fetching of creds #26

tas50 opened this issue Jul 14, 2015 · 5 comments

Comments

@tas50
Copy link
Contributor

tas50 commented Jul 14, 2015

Some of the plugins can pull from the env and others require CLI. In the long run we want to do this 100% by config, but for now we should support CLI/ENV for all plugins. Copy paste code here. Pretty simple change.

@multani
Copy link
Contributor

multani commented Dec 3, 2017

@majormoses I'd be happy to help with this issue and propose one (or several) pull request(s).

As far as I understand (cf. https://github.com/sensu-plugins/sensu-plugins-aws#authentication) that would require:

  1. to migrate all the plugins to use the AWS SDK (v2 preferably), which is tracked in Create major (breaking) release without aws sdk v1 #240
  2. remove all the command line options to which allow to set the credentials
    • there are lot of legacy code such as the aws_config method in check-elb-certs.rb (also present in other checks) to remove as well. AFAIK:
      • this overrides the imported aws_config from the Common class
      • I'm not sure how it works, since the AWS related config options are not defined anywhere
  3. I think the Common could nearly be removed unless:
    • we keep the aws_region flag in the checks which are relevant
    • in this case, we let the last bits of this Common class which sets the region.
  4. Maybe write some kind of guidelines for future checks on how to write them and handle authentication?

Obviously, that would be a major breaking change, but that would make the configuration more uniform and would be a nice simplification/cleanup of all these checks.

@majormoses
Copy link
Member

Thanks for taking a look and volunteering to help, let me try to address inline. @eheydrick correct me if I am missing anything.

to migrate all the plugins to use the AWS SDK (v2 preferably), which is tracked in #240

Yup, any work on this front is greatly appreciated

remove all the command line options to which allow to set the credentials

Yup, I'd do it in the same PR as updating to sdk v2. Example: #248

there are lot of legacy code such as the aws_config method in check-elb-certs.rb (also present in other checks) to remove as well. AFAIK

Yes, I'd remove it in the same PR as updating to sdk v2.

this overrides the imported aws_config from the Common class

Hmm I was under the impression only the checks that have been migrated to v2 had included Common which after checking the example you show there is a namespace collision.

I'm not sure how it works, since the AWS related config options are not defined anywhere

Well it basically updates those options if defined otherwise relies on the base aws config class which automatically searches for credentials from the ENV and the .credentials file. here is the documentation for this: http://docs.aws.amazon.com/sdkforruby/api/ (scroll down to "Configuration" section).

I think the Common could nearly be removed unless:
we keep the aws_region flag in the checks which are relevant

It is very relevant and 3/4 of the checks need a region.

Maybe write some kind of guidelines for future checks on how to write them and handle authentication?

If you include Common, are using aws-sdk v2, and do not have anything explicitly sending credentials, that is all that is required to set up authentication for checks.

@zbintliff
Copy link
Contributor

Just a question. Why remove the ability to specify via command line? The code you documented, allows for CLI, Env, and .aws/config. The only thing it doesn't allow (but can with a very simple change) is specifying role by cli.

@eheydrick
Copy link
Collaborator

The issue with credentials on the command line is that it's far too easy to do it insecurely. If you don't redact the keys you'll end up exposing them in logs, the API, source control, the dashboard, etc. By removing that option we avoid the exposure potential.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

6 participants