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

Provide support for AWS credentials obtained through SSO #17

Closed
mxro opened this issue Dec 4, 2021 · 13 comments
Closed

Provide support for AWS credentials obtained through SSO #17

mxro opened this issue Dec 4, 2021 · 13 comments

Comments

@mxro
Copy link
Collaborator

mxro commented Dec 4, 2021

While #3 will provide a way to consume credentials from ~/.aws/credentials this will probably not work for users logging in through SSO:

aws/aws-cli#4982

There is a node library that allows consuming these credentials:

https://github.com/ryansonshine/aws-sso-creds-helper/blob/main/src/sso-creds.ts#L48

However, it seems never versions of the JavaScript SDK also support using these credentials directly. However, I think that would require a major upgrade of the AWS SDK used by Goldstack:

See fromSSO on https://www.npmjs.com/package/@aws-sdk/credential-providers

@chrismilleruk
Copy link
Collaborator

I hit this issue today while evaluating goldstack (which looks to be great btw)

An alternate approach might be to add support for credential_process. This is neatly summarised in a comment in the same thread referenced above: aws/aws-cli#4982 (comment)

This would not require an upgrade of the AWS SDK since it exists in JS v2 as AWS.ProcessCredentials. Note that credential_process should be found in ~/.aws/config according to most other docs (not in ~/.aws/credentials)

It seems to be a valid workaround with plenty of CLI tools that fill the gap: aws-sso-creds-helper, aws-sso-util, aws-vault, aws2-wrap

Would this be a good path forward?

@mxro
Copy link
Collaborator Author

mxro commented Feb 16, 2022

Thank you that looks very promising. So with this, configuration would only need to contain the following:

{
  "users": [
    {
      "name": "dev-user",
      "type": "credentialProcess",
      "config": {
        "profile": "default"
      }
    }
  ]
}

(will need to implement that of course)

Then for this type of users it would instantiate AWS.ProcessCredentials in infraAws.ts#L282 and that should then provide a good way to get SSO credentials, correct?

And just using the profile provider would not work, correct? infraAws.ts#L262

@chrismilleruk
Copy link
Collaborator

Yes. It would probably be worth adding an option for fileName which "defaults to ~/.aws/credentials or defined by AWS_SHARED_CREDENTIALS_FILE process env var" and seems to be wrong in most cases.

My ~/.aws/config file looks something like this:

[profile goldstackApp.GoldstackAccess]
sso_start_url = https://d-1234567890.awsapps.com/start
sso_region = eu-west-1
sso_account_name = goldstackApp
sso_account_id = 123456789012
sso_role_name = GoldstackAccess
credential_process = aws-sso-util credential-process --profile goldstackApp.GoldstackAccess

So config.json might need to look something like this (assuming the config gets passed to AWS.ProcessCredentials unaltered)

{
  "users": [
    {
      "name": "dev-user",
      "type": "credentialProcess",
      "config": {
        "profile": "profile goldstackApp.GoldstackAccess",
        "fileName": "~/.aws/config",
      }
    }
  ]
}

Then for this type of users it would instantiate AWS.ProcessCredentials in infraAws.ts#L282 and that should then provide a good way to get SSO credentials, correct?

If you're taking an explicit approach, this makes good sense.

And just using the profile provider would not work, correct? infraAws.ts#L262

Just using the current implementation of profile provider doesn't seem to work for me and the error message 'Please perform aws login for the profile using the AWS CLI.' (infraAws.ts#L275) threw me off a bit.

Reading the docs, AWS.SharedIniFileCredentials expects to find aws_access_key_id and aws_secret_access_key in the INI file and therefore would ignore credential_process

If you did want to keep this under the profile provider, I think something like AWS.CredentialProviderChain could be used. e.g. https://gist.github.com/pahud/836481ae759147d3f493d3ead1f5406a

NB. I didn't see a contributor guide here and I'm not sure how to work on a submodule in the yarn workspace / pnp setup. If you can help with that, I'd be happy to either contribute a PR and/or test it with my setup here.

@mxro
Copy link
Collaborator Author

mxro commented Feb 17, 2022

Thanks for the thoughts and pointers. I have put together a draft PR: #95

A couple of questions:

  • Does the way to override the default path for the credentials file look okay? (To me a bit confusing here is that they talk about the configuration path but it would always be the path to the credentials file rather than the config file?)
  • Instead of adding a new user type, I've added an additional proprty to the profile user type, "processCredentials": true,. Does that make sense?
   {
      "name": "process",
      "type": "profile",
      "config": {
        "profile": "with-process",
        "awsDefaultRegion": "us-west-2",
        "processCredentials": true,
        "awsConfigFileName": "~/.aws/mycredentials"
      }
    }
  • Not sure how well this would work with assume-role? (The way AWS credentials are acquired really seems to be a science all to itself!)

I have also started putting together a Contribute section (#94) - but this is still WIP. Anything that stand out to you that would be worthwhile to add?

@chrismilleruk
Copy link
Collaborator

Does the way to override the default path for the credentials file look okay? (To me a bit confusing here is that they talk about the configuration path but it would always be the path to the credentials file rather than the config file?)

Yeah, it's definitely confusing. In fact I thought it was the other way around (i.e. AWS.ProcessCredentials talks about the credential path in their docs but it would always be the path to config.).

Every other doc I've seen puts the credential_process property in ~/.aws/config.

I ran some tests today and if you set AWS_SDK_LOAD_CONFIG=1, then it will search for profiles in both config & credentials. No need to provide a filename.

Instead of adding a new user type, I've added an additional proprty to the profile user type, "processCredentials": true,. Does that make sense?

Yes, this makes good sense. it's definitely feels like it should sit under profile instead of being a new user type. I also like that you can specify which provider/behaviour you want to use rather than relying on some opaque precedence rules.

Elsewhere I've noticed credential_source can be used to influence the logic in some places so naming might be better as credentialSource: "process". Leaving it blank could perhaps try a few methods?

Not sure how well this would work with assume-role?

Not sure either, I suspect it would be OK. The expected response from credential_process is very similar to response from sts assume-role. I wouldn't be surprised if many of those tools mentioned above use sts assume-role behind the scenes.

The way AWS credentials are acquired really seems to be a science all to itself!

Yes, unfortunately I'm just a lab assistant :). @benkehoe is the chief scientist in my book theres-a-better-way

@chrismilleruk
Copy link
Collaborator

I ran some tests today and if you set AWS_SDK_LOAD_CONFIG=1, then it will search for profiles in both config & credentials. No need to provide a filename.

Screenshot 2022-02-18 at 16 30 38

Screenshot 2022-02-18 at 16 31 24

@chrismilleruk
Copy link
Collaborator

chrismilleruk commented Feb 18, 2022

btw, the SSO credentials expire after 60 minutes and some of the terraform scripts can take a while to run (especially if waiting on DNS propagation). The full batch can easily take over an hour on a fresh install.

getPromise() will refresh the credentials if they are due to expire (15 second window). Passing these values to a docker container as environment vars effectively removes the ability to refresh / extend the session for the lifetime of the container.

I'll see if I can find a mechanism to allow terraform to refresh credentials from within a docker container.

@mxro
Copy link
Collaborator Author

mxro commented Feb 19, 2022

Thanks for testing things out and for your ideas!

I think "credentialsSource": "process", should work very nicely and changed the config in the PR to reflect that https://github.com/goldstack/goldstack/pull/95/files#diff-d3e9e88cc2f0ee2aad83c3b15bdd7ddc45341b6c988a2bbbe1b06e63f2947667R85

Somehow setting AWS_SDK_LOAD_CONFIG=1 led to the test to fail (disabled it for now https://github.com/goldstack/goldstack/pull/95/files#diff-e43f64484d679c0de71cca9f787c4dc4d20e104fbd0faada8b38efc275670918R84)

➤ YN0000:   ● AWS User config › Should read from AWS config in cli provider
➤ YN0000: 
➤ YN0000:     ENOENT: no such file or directory, open '/home/runner/.aws/config'
➤ YN0000: 
➤ YN0000:       486[17](https://github.com/goldstack/goldstack/runs/5256339838?check_suite_focus=true#step:14:17) | 
➤ YN0000:       486[18](https://github.com/goldstack/goldstack/runs/5256339838?check_suite_focus=true#step:14:18) |         openSync(p, flags, mode) {
➤ YN0000:     > 486[19](https://github.com/goldstack/goldstack/runs/5256339838?check_suite_focus=true#step:14:19) |           return this.realFs.openSync(npath.fromPortablePath(p), flags, mode);
➤ YN0000:             |                              ^
➤ YN0000:       486[20](https://github.com/goldstack/goldstack/runs/5256339838?check_suite_focus=true#step:14:20) |         }
➤ YN0000:       486[21](https://github.com/goldstack/goldstack/runs/5256339838?check_suite_focus=true#step:14:21) | 
➤ YN0000:       486[22](https://github.com/goldstack/goldstack/runs/5256339838?check_suite_focus=true#step:14:22) |         async opendirPromise(p, opts) {
➤ YN0000: 
➤ YN0000:       at NodeFS.openSync (../../../../.pnp.cjs:48619:30)
➤ YN0000:       at makeCallSync.subPath.subPath (../../../../.pnp.cjs:51771:34)
➤ YN0000:       at ZipOpenFS.makeCallSync (../../../../.pnp.cjs:52776:32)
➤ YN0000:       at ZipOpenFS.openSync (../../../../.pnp.cjs:51768:[23](https://github.com/goldstack/goldstack/runs/5256339838?check_suite_focus=true#step:14:23))
➤ YN0000:       at VirtualFS.openSync (../../../../.pnp.cjs:49232:[30](https://github.com/goldstack/goldstack/runs/5256339838?check_suite_focus=true#step:14:30))
➤ YN0000:       at PosixFS.openSync (../../../../.pnp.cjs:492[32](https://github.com/goldstack/goldstack/runs/5256339838?check_suite_focus=true#step:14:32):30)
➤ YN0000:       at URLFS.openSync (../../../../.pnp.cjs:49232:30)
➤ YN0000:       at NodeFS.readFileSync (../../../../.pnp.cjs:49119:30)

https://github.com/goldstack/goldstack/runs/5256339838?check_suite_focus=true

Seems like it was looking up the default file location rather than the one provided in the constructor. Will try to investigate further tomorrow. Agreed it seems to make more sense to put the credentials_process into the config since it's just config and not a credential!

@chrismilleruk
Copy link
Collaborator

Somehow setting AWS_SDK_LOAD_CONFIG=1 led to the test to fail
Seems like it was looking up the default file location rather than the one provided in the constructor. Will try to investigate further tomorrow.

Ah yes, I did read the code path in IniLoader (the underlying file loader) and it seems to ignore the file name property if AWS_SDK_LOAD_CONFIG is set.

I suspect that if you set AWS_CONFIG_FILE then it will work.

Chris

@mxro
Copy link
Collaborator Author

mxro commented Feb 20, 2022

Got it working with both using credentials and config files, see https://github.com/goldstack/goldstack/pull/95/files#diff-d3e9e88cc2f0ee2aad83c3b15bdd7ddc45341b6c988a2bbbe1b06e63f2947667R110. Had to do some fiddinling around with the environment variables to make it work 😱 https://github.com/goldstack/goldstack/pull/95/files#diff-e43f64484d679c0de71cca9f787c4dc4d20e104fbd0faada8b38efc275670918R83 So happy for any better ideas.

I have merged and released what is there so far, which should now support using process credentials with new templates (or by upgrading a package to the latest version of the template dependency).

Also updated the configuration with a section on Process Credentials.

I raised a seperate issue for tracking the issue you identified regarding long running infra operations: #99

I think this should (if it works!) provide at least some way to use an SSO-based login for local development? Thanks for all your work so far!!!

@chrismilleruk
Copy link
Collaborator

Thanks for the quick response @mxro, this is working for me now (with a workaround**) 🚀

I found some typos and other minor inconsistencies so I'll put a review on the PR for you.

I still can't make changes locally using yarn link * or yarn patch but I made a bit of progress and I'll raise a few issues related to that. Happy to be added as a collaborator if that's a useful path.

** I had to either provide awsConfigFileName with the default ~/.aws/config location, OR execute yarn infra with a prefix (e.g. AWS_SDK_LOAD_CONFIG=1 yarn infra init dev) to get this working. I'll add this to the PR review also!

@mxro
Copy link
Collaborator Author

mxro commented Feb 21, 2022

That's great to hear. Yeah a lots of bits and pieces of logic esp around the env variables, so definitely need to tweak this a bit to make it work reliabily. Thanks for your help. Started a new PR based on the comments #102 - but still WIP for now.

** I had to either provide awsConfigFileName with the default ~/.aws/config location, OR execute yarn infra with a prefix (e.g. AWS_SDK_LOAD_CONFIG=1 yarn infra init dev) to get this working.

Will want to get this working but not sure if the new PR above would resolve this.

Also added you as a collaborator. This should enable you to create and contribute to branches and the CI should run on them I hope.

@mxro
Copy link
Collaborator Author

mxro commented Mar 2, 2022

Does this work now and can be closed?

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

2 participants