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

What's the deal with credentials. It's voodoo. #1276

Closed
cmawhorter opened this issue Dec 20, 2016 · 15 comments
Closed

What's the deal with credentials. It's voodoo. #1276

cmawhorter opened this issue Dec 20, 2016 · 15 comments
Labels
feature-request A feature should be added or improved. needs-major-version Can only be considered for the next major release

Comments

@cmawhorter
Copy link

Today I'm spending my latest hour of time debugging an issue relating to the way aws sdk loads credentials. Apart from the swallowing of errors, this is my single biggest pain point with the js aws sdk.

It seems that require('aws-sdk') does some magic to load credentials, but it's a very bad magician. It' more... "is this your card" than "i just made the statue of liberty disappear".

This is the only way I've found to reliably use aws-sdk across all environments (tests, browserify/webpack, lambda, ec2, etc.).

var s3 = new AWS.S3();
module.exports = { 
  put: function() { 
    s3.put(...); 
  } 
}; 

// becomes 

module.exports = { 
  put: function() { 
    var s3 = new AWS.S3();
    s3.put(...); 
  } 
}; 

And now, throw in the async nature of cognito credential loading and mixed-credential environments, and the time I've spent working through problems around loading credentials in aws sdk can be measured in the tens of hours.

Please, please, please give me var aws = new AWS({ ...config... }) and deprecate the global AWS object.

AWS.config.update is badddddddddd. It's global, but it isn't.

var AWS = require('aws-sdk')
var s3 = new AWS.S3();
console.log('1. s3 region', s3.config.region); // us-east-1
AWS.config.update({ region: 'us-west-2' })
console.log('2. s3 region', s3.config.region); // us-east-1
var s3 = new AWS.S3();
console.log('3. s3 region', s3.config.region); // us-west-2

Hate your job? Sprinkle AWS.config.update's throughout the codebase and watch as your team pulls their hair out.

@jeskew
Copy link
Contributor

jeskew commented Dec 20, 2016

Hi @cmawhorter,

I understand your frustration, but deprecating the global configuration object and requiring users to instantiate AWS objects would be a pretty big breaking change. It would also be a breaking change to allow AWS.config.update to modify already instantiated service objects (as documented in our developer guide) What you can do, on the other hand, is to instantiate individual service objects with their configuration passed in as an argument rather than using AWS.config. For example, your second code snippet could be rewritten to explicitly specify the desired region on each instance of AWS.S3:

var AWS = require('aws-sdk');
var s3_east = new AWS.S3({region: 'us-east-1'});
s3_west = new AWS.S3({region: 'us-west-2'});

Any option that can be set via AWS.config.update can instead be passed in as a constructor option to a service object, and that pattern might suit your needs better than AWS.config.update.

@cmawhorter
Copy link
Author

cmawhorter commented Jan 4, 2017

I'd like to see AWS.config.update deprecated entirely and not become even more global. I understand it'd be a breaking change, but in my weightless opinion, it's a worthwhile one.

The way the js aws sdk loads credentials is... unique. I can't think of any other node lib that does anything close to this.

Any option that can be set via AWS.config.update can instead be passed
in as a constructor option to a service object, and that pattern might suit
your needs better than AWS.config.update.

In practice, I've found this to not always be true. I've seen some mighty weird things around loading credentials.

I've implemented with the constructor as you describe and inspect the object and confirm it's correctly set, only to have it still claim the region is missing or make it to the wrong region.

I have no idea how that is even possible, which is why I opted to always instantiate a new service client at the time of using. That was the only thing I've found that is guaranteed to work.

Is it possible to log where the credentials are being pulled from or better yet -- disable all credential loading magic? require('aws-sdk').ICanLoadMyOwnCredentials_andalso_disable_all_globals

Edit: I found #710 which seems to at least partially describe what I've seen most recently. Also #880. Perhaps there is some lingering weirdness here? My problems weren't limited to ddb in the past, but lately, they've all been ddb related.

Also -- is it possible a node dependency is using a diff version of aws and the two different versions are somehow sharing state or something?

@jeskew
Copy link
Contributor

jeskew commented Jan 4, 2017

Getting rid of AWS.config.update isn't something we could do without a major version bump, but I'll mark this issue as a feature request to make sure we keep this feedback in mind when we do get an opportunity to make breaking changes.

Also -- is it possible a node dependency is using a diff version of aws and the two different versions are somehow sharing state or something?

When loaded in node, the SDK exports an AWS object, so each installed version of the SDK would maintain its own state. The default credential loader, however, relies on process and machine state to determine which credentials to use to sign a request: it checks environment variables, which would be shared by all installed versions of the SDK running in the same shell or process; it then checks for file-based credentials in a known location (~/.aws/credentials), which would be shared by all shells and processes run as a given user; and it then checks for instance metadata credentials, which would be shared by all users on a given EC2 instance or ECS container.

@jeskew jeskew added the feature-request A feature should be added or improved. label Jan 4, 2017
@cmawhorter
Copy link
Author

I think what I'm seeing is a bug related to a race maybe.

I've recently started using a tool for deployment that requires AWS_PROFILE be set. I'm a little murky on the internals of how aws sdk handles loading, but my gut says because that var is set, it begins to async load ~/.aws/credentials and clobbers my sync-set entries.

e.g.

export AWS_PROFILE=whatever
node script.js
// script.js
var AWS = require('aws-sdk'); // detects AWS_PROFILE
AWS.config.update({ ... });

A race would explain the intermittent nature of what I'm seeing. Though it doesn't explain why in the past, I've had an s3 client object config say it was set to one region, but ended up making requests against another, and throwing an error that the bucket didn't exist.

At any rate. I'm just going to be putting this as the first line in the entry point for my aws dependent code:

if (process.env.NODE_ENV === 'development') {
  Object.keys(process.env).forEach(key => 0 === key.indexOf('AWS_') ? delete process.env[key] : null);
}
var AWS = require('aws-sdk');

@jeskew jeskew added the needs-major-version Can only be considered for the next major release label Jan 19, 2017
@sedouard
Copy link

sedouard commented Jan 31, 2017

I don't think removing AWS.config is necessary. You can keep it and instead offer a configuration to the service constructor. For example:

var ec2Client = new AWS.EC2(awsConfig)

Where awsConfig is bascially the AWS.config global object global class but its settings will override that of the global AWS.config. This way current users are not broken, and people who want isolation in their client instances can have that too.

Otherwise, the idea of managing multiple subscriptions in-process becomes significantly more complicated. Currently today, if we asynchronously use multiple AWS credentials, we'll mix up EC2 instances between different subscriptions because one configuration bleeds over for another client.

Why would you allow for dynamically instantiated clients (new AWS.EC2()), but force them to use a global configuration? You may as well only have 1 singleton client...right?

@chrisradek
Copy link
Contributor

@sedouard
You can pass in configuration into the service clients when you instantiate them.

Taking a look at the EC2 constructor as an example, you'll notice that the options parameter is nearly identical to AWS.config. Really the only difference is that AWS.config allows you to also add service-specific configuration, whereas that's unnecessary when passing configuration directly to a service client.

Behind the scenes, the configuration passed to a service client is merged on top of a copy of AWS.config when a service client is instantiated. If you want to have different credentials for each AWS.EC2 client, you just need to provide them in the options you pass into the constructor.

@cmawhorter
Copy link
Author

cmawhorter commented Feb 16, 2017

I think I finally figured out the problem with the voodoo:

  • I had a preexisting AWS_PROFILE existing in my env from jumping between projects
  • Even though I was specifying the credentials on run, require('aws-sdk') would detect/use AWS_PROFILE (i think. even from browsing aws-sdk code i'm still not entirely sure. GOTO voodoo)
  • The error message being output (sometimes) talked about invalid credentials. This is because the absolute fallback of aws-sdk by default is to call an aws http endpoint, which times out and outputs an invalid credentials error. (other aws-sdk issues exist about this)

Does AWS_PROFILE get higher priority than AWS.config({ ... })?

I don't know. And that's my problem.

Instead of magic on require('aws-sdk'), I should specifically need to call AWS.loadCredentialsFromEnv() or something.

That auto-loading behavior of aws-sdk has bit me numerous times during dev. With code being deployed to wrong environments/accounts and seemingly valid credentials being rejected for some unknown reason.

Behind the scenes, the configuration passed to a service client
is merged on top of a copy of AWS.config when a service client
is instantiated

If someone is using AWS.config somewhere, it's very difficult to figure out where/what/when that global method is being invoked and can lead to a race with credentials and different configs being used depending on which async code ran an AWS.config when.

In practice, I never use AWS.config. But dependencies and more junior developers sometimes do. (Largely because of the abundance of docs and blog posts out there saying to use AWS.config). And that fact leads to extremely difficult to debug problems.

And if you're all-in on using AWS.config, there is no way to tell an EC2 constructor for example, to not merge the global AWS.config. So even if you're doing new AWS.EC2({ ...}), unless you're explicitly listing an exhaustive list of options, there is no way to prevent AWS.config from trickling down.

Edit: Added inverse example

@joshgoodwin
Copy link

I'd like to add my anger and frustration to this thread. I just lost half of a day I couldn't afford to lose hunting this garbage down. How many 10s's of thousands of man hours has this dumb decision (AWS.config overriding other AWS.config) cost us? I don't care if it's a breaking change, you have namespaced versions of your SDK. Make the change.

@diego-inflightvr
Copy link

I'd also like to add my anger and frustration to this thread (I believe this is the right place to do it, but if not, please do tell me).
I spent all the day trying to fix an Expired Credentials issue just because I cannot update service objects credentials.

I've tried everything... From trying to update the credentials in the global AWS config, to keeping a reference to the credentials object passed to the service object so I can refresh them with a new token.

This is my situation:

  1. I'm using Amazon Cognito with the approach of Developer Authenticated Identities. I think this an architecture/solution recommended by Amazon to avoid distributing long-term credentials in client apps
  2. My API gets a token and an IdentityId by calling getOpenIdTokenForDeveloperIdentity (valid for no more than 24hs)
  3. These two things are sent to my client app so it can create a AWS.CognitoIdentityCredentials that will be used later by an AWS.S3 object to download some files

The problem is that the max duration of the token in step #2 is 24hs. So, when my client application creates an AWS.S3 object with some CognitoIdentityCredentials that use this token, this S3 object is doomed to last no more than 24hs (just because I cannot update/refresh the CognitoIdentityCredentials that the S3 is using).

Am I right? I still think that maybe I'm missing something but I've done my research (i.e. Amazon Doc see last section "Immutable Configuration Data") and I cannot find where my error is.
I cannot believe that I'm following an approach recommended by Amazon that doesn't allow me download a file from S3 for more than 24hs...

Thanks in advance to anyone that can help me...

@donspaulding
Copy link

Got bit by this today. Here was our use-case, simplified:

class OurClass {
    constructor() {
        this.options = optionsFromProcessDotEnvOrWherever;
        this.doSomethingWithS3();
    }

    doSomethingWithS3() {
        s3 = this.s3Config();
        // does something with s3
        // throws: InvalidAccessKeyId: The AWS Access Key Id you provided does not exist in our records.
    }

    s3Config() {
        const AWS = require('aws-sdk');
        // How NOT to configure aws-sdk endpoints:
        AWS.config = new AWS.Config();
        AWS.config.accessKeyId = this.options.s3.id;
        AWS.config.secretAccessKey = this.options.s3.secret;
        AWS.config.region = this.options.s3.region;
        return new AWS.S3(AWS.config);

        // Presumably, the reason the 5 lines above don't work is that we're
        // setting configuration values on the "global" AWS.config.
        // aws-sdk later comes along and clobbers those values with
        // values found in the environment (~/.aws/credentials). 
    }
}

In order to fix this, we had to change the return of s3Config() to something like this:

        return new AWS.S3({
            accessKeyId: this.options.s3.id,
            secretAccessKey: this.options.s3.secret,
            region: this.options.s3.region,
        });

Basically, you can't bind anything to the global config, because there's a race to write to it and AWS will blow away whatever it finds there whenever it finishes loading up the credentials.

Count me as one more who was caught off guard by the code above not working as expected. I'm rarely an advocate for backwards-incompatible changes, but race conditions for your main configuration seems like much more of a bug than a feature.

@trunderw
Copy link

To the original poster -
The problem is the order of the operations you're doing. The creation of the s3 object instance is based on the last config you provided, then it holds onto that config for subsequent calls from that instance. In your example with some of my comments added to yours:

var AWS = require('aws-sdk')
var s3 = new AWS.S3(); //s3 object created with whatever config is default for you
console.log('1. s3 region', s3.config.region); // us-east-1 //**looks like it's us-east-1
AWS.config.update({ region: 'us-west-2' })  //**this will affect future S3 objects but not the existing s3 isntance!
console.log('2. s3 region', s3.config.region); // us-east-1 //**still using original s3 object created in line 2 
var s3 = new AWS.S3(); //**updates the config
console.log('3. s3 region', s3.config.region); // us-west-2 //**uses the new s3 object with the new config

This is working reliably for me when you take this into consideration. Your workaround described initially is essentially enforcing the correct order of operations for you.

@cmawhorter
Copy link
Author

so, first of all... i discovered aws-sdk v3 the other day and was extremely happy to see they burned aws-sdk v2 to the ground. this lib is garbage and has been the source of so much pain and suffering for so many. you're not a js dev until you've posted a rant here at aws/aws-sdk-js/issues.

and @trunderw that might work in your environment but i would hesitate to say it's universal because of the variety of ways AWS can infer credentials from the env and the variety of ways some jr dev (or yourself a year later) can mess that up for everyone.

it's such a mess i've just enforced always providing a region to service constructors and you must allow AWS to get creds from env. period. this means AWS_PROFILE on the dev machine and letting aws-sdk do its thing on ec2/lambda. once i did that all these issues disappeared. if for some reason you can't lean on iam roles and have to juggle creds yourself... god help you.

the other issue is the i-wish-i-was-windows vague error messages aws-sdk spits out along with obscuring the real error/stack behind the internal state machine bs. meaning -- if for whatever reason AWS fails to load any local credentials it'll fall back to trying to get them via remote http. this leads to a totally unrelated (but seemingly related) error message about an http timeout that is easy to mistake as being because your endpoint/region is wrong. (there is another issue about this somewhere around here).

i can't tell you how many times i've had to resort to console.log(1), 2, 3, 4, ... to figure out why/where something lead to an error that aws-sdk has swallowed. it's probably days of my life at this point which just makes me sad.

and with that. i'm done here. here's to hoping MS/azure's wins continue to push AWS into action because there sure as hell wasn't any action until azure showed up.

@trunderw
Copy link

@cmawhorter: given the points about the environment maybe clarifying what mine is will help someone in the future. My particular S3-focused (with occasional ECR) app is running by itself on node in a docker container on a kubernetes cluster. There's nothing else running in the pod so from that perspective, it should avoid config creep from jr or any other well-intentioned developers. Maybe that is the only environment that the aws js sdk is certified to work with =)
I will be following v3 of the sdk so thanks for pointing that out.

@ajredniwja
Copy link
Contributor

Hey @cmawhorter,

Is this still an ongoing feature-request after #1317 ?

Will go ahead and close this issue. Please re-open/reach out if you have any additional questions.

@lock
Copy link

lock bot commented Nov 20, 2019

This thread has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs and link to relevant comments in this thread.

@lock lock bot locked as resolved and limited conversation to collaborators Nov 20, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
feature-request A feature should be added or improved. needs-major-version Can only be considered for the next major release
Projects
None yet
Development

No branches or pull requests

9 participants