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

Toolkit: bail out early if credentials are not defined #130

Closed
eladb opened this issue Jun 19, 2018 · 14 comments · Fixed by #724
Closed

Toolkit: bail out early if credentials are not defined #130

eladb opened this issue Jun 19, 2018 · 14 comments · Fixed by #724
Assignees
Labels
feature-request A feature should be added or improved.

Comments

@eladb
Copy link
Contributor

eladb commented Jun 19, 2018

Seems like currently even if there are no credentials defined in the default chain, the toolkit will still execute Looking up default account ID from STS which will time out and only then a message is displayed that credentials are not found.

We should be able to bail out early if there are no credentials defined in the chain (we should make sure that plugins can still do their job)

@RomainMuller
Copy link
Contributor

@eladb how do we know there is no credentials? Is there a SDK/client way to test upfront? Because my understanding is the way to do is try to make a call and realize it fails... And if that takes long on the STS call, it's because it tries to contact EC2 Metadata Service, which can be client-disabled...

@rix0rrr
Copy link
Contributor

rix0rrr commented Jun 19, 2018

Yeah the timeout happens in determining whether there are credentials available. Specifically, whether there are instance creds.

Once there aren't, the actual call to STS is not even attempted. (But the logging can't tell so it looks misleading)

@rix0rrr
Copy link
Contributor

rix0rrr commented Jun 19, 2018

We can configure a credential chain w/o instance creds, but then it won't work on EC2/CodeBuild. Add a switch?

@RomainMuller
Copy link
Contributor

On machines that are not EC2, one can add this to their shell profile to disable EC2 metadata service lookups "globally" (at least for the JS SDK):

export AWS_EC2_METADATA_DISABLED=1

@rix0rrr
Copy link
Contributor

rix0rrr commented Jun 19, 2018

Yeah, sure, but that's not the default. Just wondering out loud if it makes sense to invert the default in our case

@RomainMuller
Copy link
Contributor

I disagree. People may well use an EC2 instance as their primary environment & rely on the instance profile. The behavior I want to be aiming for is "same as SDK / CLI", so I would like to deviate form the defaults as little as possible.

@eladb
Copy link
Contributor Author

eladb commented Jun 20, 2018

I am actually realizing that in order to obtain the account ID we are issuing the STS call in every toolkit invocation, which is terribly sad. Maybe we can cache it somehow...

For example, can we instantiate the default credentials provider in the toolkit (before the STS cal) and request credentials.

  • If there are no credentials - bail out
  • if there are credentials, use the access key as a key into an on-disk cache from key to account ID. If there is no entry in the cache, fetch account ID from STS.

@RomainMuller
Copy link
Contributor

If doing the disk cache, consider that a lot of people are going to be dependent on temporary credentials (EC2 instance profile, federated credentials, ...). Don't want that file to grow forever.

@eladb
Copy link
Contributor Author

eladb commented Jun 20, 2018

Don't want that file to grow forever.
Good point

@rix0rrr
Copy link
Contributor

rix0rrr commented Jun 22, 2018

People may well use an EC2 instance as their primary environment & rely on the instance profile

They may, but I honestly don't expect this to be a common case.

Anyhoo, on-disk cache of size 1 fixes the slowness, so we can totally do that.

@RomainMuller
Copy link
Contributor

Cloud9?

@rix0rrr
Copy link
Contributor

rix0rrr commented Jun 22, 2018

Can't imagine they rely on instance profile creds. But maybe they do.

@rix0rrr
Copy link
Contributor

rix0rrr commented Aug 16, 2018

This might be useful for an optimization:

https://docs.aws.amazon.com/AWSEC2/latest/UserGuide/identify_ec2_instances.html
https://docs.aws.amazon.com/AWSEC2/latest/WindowsGuide/identify_ec2_instances.html

On Lambda we get environment variables prepopulated.

Don't know about ECS.

@0xdevalias
Copy link
Contributor

0xdevalias commented Sep 14, 2018

I've just run into this issue myself (from #702), I tend not to have default creds set, but use a profile (which I hadn't specified), takes forever to eventually get the timeout from 169.254.169.254 being unreachable:

Resolving default credentials
Unable to determine the default AWS account (did you configure "aws configure"?): { Error: connect ETIMEDOUT 169.254.169.254:80
    at TCPConnectWrap.afterConnect [as oncomplete] (net.js:1113:14)
  errno: 'ETIMEDOUT',
  code: 'ETIMEDOUT',
  syscall: 'connect',
  address: '169.254.169.254',
  port: 80 }
Setting "aws:cdk:toolkit:default-account" context to undefined
node bin/poc-aws-cdk.js 'base64:eyJ0eXBlIjoibGlzdCIsImNvbnRleHQiOnsiYXdzOmNkazp0b29sa2l0OmRlZmF1bHQtcmVnaW9uIjoiYXAtc291dGhlYXN0LTIifX0='

A couple of questions:

  • As mentioned above, could we detect in a better way when in a 'cloud like' environment, and only then attempt to use the 'magic IP'
  • Could we turn down the timeout on the connect request to something smaller (eg. 5sec), which would presumably be more than enough to hit an 'internal address' when running on a cloud-like environment
  • How does a command such as aws sts get-caller-identity handle this sort of situation? As for me, it figures out instantly that I don't have creds defined:
⇒  aws sts get-caller-identity
Unable to locate credentials. You can configure credentials by running "aws configure".

rix0rrr pushed a commit that referenced this issue Sep 17, 2018
Automatically detect whether we're on an EC2 instance and only add
looking up metadata credentials if that appears to be true. Add
`--instance`, `--no-instance` command-line arguments to override
the guessing if it happens to be wrong.

This will fix long hangs for people that happen to be on machines
where the metadata service address happens to be routable or blackholed,
such as observed in #702.

Fixes #130.
rix0rrr pushed a commit that referenced this issue Sep 17, 2018
Automatically detect whether we're on an EC2 instance and only add
looking up metadata credentials if that appears to be true. Add
`--instance`, `--no-instance` command-line arguments to override
the guessing if it happens to be wrong.

This will fix long hangs for people that happen to be on machines
where the metadata service address happens to be routable or blackholed,
such as observed in #702.

Fixes #130.
rix0rrr added a commit that referenced this issue Sep 17, 2018
Automatically detect whether we're on an EC2 instance and only add
looking up metadata credentials if that appears to be true. Add
`--ec2creds`, `--no-ec2creds` command-line arguments to override
the guessing if it happens to be wrong.

This will fix long hangs for people that happen to be on machines
where the metadata service address happens to be routable or blackholed,
such as observed in #702.

Fixes #130.
eladb pushed a commit that referenced this issue Sep 20, 2018
__NOTICE__: This release includes a framework-wide [__breaking
change__](#712) which changes the type
of all the string resource attributes across the framework. Instead of using
strong-types that extend `cdk.Token` (such as `QueueArn`, `TopicName`, etc), we
now represent all these attributes as normal `string`s, and codify the tokens
into the string (using the feature introduced in [#168](#168)).

Furthermore, the `cdk.Arn` type has been removed. In order to format/parse ARNs,
use the static methods on `cdk.ArnUtils`.

See motivation and discussion in [#695](#695).

* **cfn2ts:** use stringified tokens for resource attributes instead of strong types ([#712](#712)) ([6508f78](6508f78)), closes [#518](#518) [#695](#695) [#744](#744)
* **aws-dynamodb:** Attribute type for keys, changes the signature of the `addPartitionKey` and `addSortKey` methods to be consistent across the board. ([#720](#720)) ([e6cc189](e6cc189))
* **aws-codebuild:** fix typo "priviledged" -> "privileged

* **assets:** cab't use multiple assets in the same stack ([#725](#725)) ([bba2e5b](bba2e5b)), closes [#706](#706)
* **aws-codebuild:** typo in BuildEnvironment "priviledged" -> "privileged     ([#734](#734)) ([72fec36](72fec36))
* **aws-ecr:** fix addToResourcePolicy ([#737](#737)) ([eadbda5](eadbda5))
* **aws-events:** ruleName can now be specified ([#726](#726)) ([a7bc5ee](a7bc5ee)), closes [#708](#708)
* **aws-lambda:** jsii use no long requires 'sourceAccount' ([#728](#728)) ([9e7d311](9e7d311)), closes [#714](#714)
* **aws-s3:** remove `policy` argument ([#730](#730)) ([a79190c](a79190c)), closes [#672](#672)
* **cdk:** "cdk init" java template is broken ([#732](#732)) ([281c083](281c083)), closes [#711](#711) [aws/jsii#233](aws/jsii#233)

* **aws-apigateway:** new API Gateway Construct Library ([#665](#665)) ([b0f3857](b0f3857))
* **aws-cdk:** detect presence of EC2 credentials ([#724](#724)) ([8e8c295](8e8c295)), closes [#702](#702) [#130](#130)
* **aws-codepipeline:** make the Stage insertion API in CodePipeline more flexible ([#460](#460)) ([d182818](d182818))
* **aws-codepipeline:** new "Pipeline#addStage" convenience method ([#647](#647)) ([25c9fa0](25c9fa0))
* **aws-rds:** add support for parameter groups ([#729](#729)) ([2541508](2541508)), closes [#719](#719)
* **docs:** add documentation for CDK toolkit plugings ([#733](#733)) ([965b918](965b918))
* **dependencies:** upgrade to [jsii 0.7.6](https://github.com/awslabs/jsii/releases/tag/v0.7.6)
eladb pushed a commit that referenced this issue Sep 20, 2018
* v0.9.2

__NOTICE__: This release includes a framework-wide [__breaking
change__](#712) which changes the type
of all the string resource attributes across the framework. Instead of using
strong-types that extend `cdk.Token` (such as `QueueArn`, `TopicName`, etc), we
now represent all these attributes as normal `string`s, and codify the tokens
into the string (using the feature introduced in [#168](#168)).

Furthermore, the `cdk.Arn` type has been removed. In order to format/parse ARNs,
use the static methods on `cdk.ArnUtils`.

See motivation and discussion in [#695](#695).

* **cfn2ts:** use stringified tokens for resource attributes instead of strong types ([#712](#712)) ([6508f78](6508f78)), closes [#518](#518) [#695](#695) [#744](#744)
* **aws-dynamodb:** Attribute type for keys, changes the signature of the `addPartitionKey` and `addSortKey` methods to be consistent across the board. ([#720](#720)) ([e6cc189](e6cc189))
* **aws-codebuild:** fix typo "priviledged" -> "privileged

* **assets:** cab't use multiple assets in the same stack ([#725](#725)) ([bba2e5b](bba2e5b)), closes [#706](#706)
* **aws-codebuild:** typo in BuildEnvironment "priviledged" -> "privileged     ([#734](#734)) ([72fec36](72fec36))
* **aws-ecr:** fix addToResourcePolicy ([#737](#737)) ([eadbda5](eadbda5))
* **aws-events:** ruleName can now be specified ([#726](#726)) ([a7bc5ee](a7bc5ee)), closes [#708](#708)
* **aws-lambda:** jsii use no long requires 'sourceAccount' ([#728](#728)) ([9e7d311](9e7d311)), closes [#714](#714)
* **aws-s3:** remove `policy` argument ([#730](#730)) ([a79190c](a79190c)), closes [#672](#672)
* **cdk:** "cdk init" java template is broken ([#732](#732)) ([281c083](281c083)), closes [#711](#711) [aws/jsii#233](aws/jsii#233)

* **aws-apigateway:** new API Gateway Construct Library ([#665](#665)) ([b0f3857](b0f3857))
* **aws-cdk:** detect presence of EC2 credentials ([#724](#724)) ([8e8c295](8e8c295)), closes [#702](#702) [#130](#130)
* **aws-codepipeline:** make the Stage insertion API in CodePipeline more flexible ([#460](#460)) ([d182818](d182818))
* **aws-codepipeline:** new "Pipeline#addStage" convenience method ([#647](#647)) ([25c9fa0](25c9fa0))
* **aws-rds:** add support for parameter groups ([#729](#729)) ([2541508](2541508)), closes [#719](#719)
* **docs:** add documentation for CDK toolkit plugings ([#733](#733)) ([965b918](965b918))
* **dependencies:** upgrade to [jsii 0.7.6](https://github.com/awslabs/jsii/releases/tag/v0.7.6)
@srchase srchase added feature-request A feature should be added or improved. and removed enhancement labels Jan 3, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature-request A feature should be added or improved.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants