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

AWS credentials not being read from ~/.aws/credentials #27

Closed
blablabla42 opened this issue Nov 19, 2018 · 6 comments
Closed

AWS credentials not being read from ~/.aws/credentials #27

blablabla42 opened this issue Nov 19, 2018 · 6 comments

Comments

@blablabla42
Copy link

Hi - we're trying to use pganalyze and have been unsuccessful in configuring it to read AWS credentials from ~/.aws/credentials [0].

This is a well known location that's supported by default by the aws-sdk-go [1]. It looks like instead https://github.com/pganalyze/collector/blob/master/util/awsutil/amazon.go#L18 is being used to read credentials. Why is that?

Would it be possible to use the default credentials chain?

[0] https://docs.aws.amazon.com/cli/latest/userguide/cli-config-files.html
[1] https://docs.aws.amazon.com/sdk-for-go/v1/developer-guide/configuring-sdk.html

@blablabla42
Copy link
Author

I confirmed with following diff that using the default credentials provider exhibited the expected behaviour.

diff --git a/util/awsutil/amazon.go b/util/awsutil/amazon.go
index 5bf6f68..aa60c46 100644
--- a/util/awsutil/amazon.go
+++ b/util/awsutil/amazon.go
@@ -2,25 +2,12 @@ package awsutil
 
 import (
        "github.com/aws/aws-sdk-go/aws"
-       "github.com/aws/aws-sdk-go/aws/credentials"
-       "github.com/aws/aws-sdk-go/aws/credentials/ec2rolecreds"
-       "github.com/aws/aws-sdk-go/aws/ec2metadata"
        "github.com/aws/aws-sdk-go/aws/session"
        "github.com/pganalyze/collector/config"
 )
 
 func GetAwsSession(config config.ServerConfig) *session.Session {
-       var creds *credentials.Credentials
-
-       if config.AwsAccessKeyID != "" {
-               creds = credentials.NewStaticCredentials(config.AwsAccessKeyID, config.AwsSecretAccessKey, "")
-       } else {
-               creds = credentials.NewCredentials(&ec2rolecreds.EC2RoleProvider{
-                       Client: ec2metadata.New(session.New()),
-               })
-       }
-
-       return session.New(&aws.Config{Credentials: creds, Region: aws.String(config.AwsRegion)})
+       return session.New(&aws.Config{Region: aws.String(config.AwsRegion)})
 }

@lfittl
Copy link
Member

lfittl commented Nov 30, 2018

Hi @blablabla42 - So just to confirm, you would then want to place the AWS credentials in the home directory of the pganalyze user?

(in the typical package-based setup there would be a dedicated pganalyze user)

I'm okay with supporting the config file here, however we'd need to maintain compatibility with specifying the access key in the config variables (i.e. not remove that logic altogether)

@blablabla42
Copy link
Author

So just to confirm, you would then want to place the AWS credentials in the home directory of the pganalyze user?

Yeap, that's correct.

I'm okay with supporting the config file here, however we'd need to maintain compatibility with specifying the access key in the config variables (i.e. not remove that logic altogether)

Correct, the diff above doesn't address that. I think it's doable by modifying the default credentials chain to add the newStaticCredentials instead of overwriting the credentials chain.

@mgood
Copy link

mgood commented Jan 7, 2019

It looks like if you just delete the else block it will leave cred = nil, which should have the intended effect.

Can you please also update the vendored SDK to support the latest credential chain? We're running the collector on ECS, so this would allow it to pick up the task role automatically.

@mgood
Copy link

mgood commented Jan 7, 2019

To clarify, using the default credential chain would not only allow using ~/.aws/credentials, but also other standard ways the AWS SDK detects the credentials, including the EC2 and ECS metadata (which have different endpoints), and other mechanisms AWS may add in the future. So, this would be an extremely useful update.

@lfittl lfittl closed this as completed in 3b9a829 Jan 11, 2019
@lfittl
Copy link
Member

lfittl commented Jan 11, 2019

@mgood @blablabla42 Committed that change to master in 199630b and 3b9a829 - will make a new release soon that includes that.

Thanks for your input on this!

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

No branches or pull requests

3 participants