thomasmichaelwallace / serverless-better-credentials

Better AWS credentials resolution plugin for serverless
MIT License
58 stars 10 forks source link

Add missing providers from the SDK default chain #32

Closed tim-hutchinson-kc closed 11 months ago

tim-hutchinson-kc commented 1 year ago

Is your feature request related to a problem? Please describe. There are several providers in the default AWS SDK provider chain that don't exist in this implementation, which causes users leveraging them to experience errors (https://github.com/thomasmichaelwallace/serverless-better-credentials/issues/24), since those methods are never tried

Describe the solution you'd like The default provider chain in the AWS SDK is

AWS.EnvironmentCredentials('AWS')
AWS.EnvironmentCredentials('AMAZON')
AWS.SsoCredentials
AWS.SharedIniFileCredentials
AWS.ECSCredentials
AWS.ProcessCredentials
AWS.TokenFileWebIdentityCredentials
AWS.EC2MetadataCredentials

ignoring the EnvironmentCredentials for the moment, currently what's implemented here is

AWS.SharedIniFileCredentials
SsoCredentials # The local implementation
AWS.ProcessCredentials

I'd propose adding the remaining providers to the chain:

AWS.ECSCredentials
# ProcessCredentials is already added
AWS.TokenFileWebIdentityCredentials
AWS.EC2MetadataCredentials

Additional context I started implementing this on my end and had some questions about the current setup.

  1. It looks like the precedence of SSO vs SharedIniFile is flipped from the default in the current implementation. Should that order be preserved?
  2. It seems that for each potential profile value, all the possible providers are added to the chain. I would have thought that profile could only ever be one value, taking a precedence of resolution, and then that value is used for the providers that use it?
let profile = this.options['aws-profile'] || process.env[`AWS_${stageUpper}_PROFILE`] || process.env.AWS_PROFILE || this.serverless.service.provider.profile || process.env.AWS_DEFAULT_PROFILE

params = { profile }
this.chain.push(SsoProvider(params))

Is that just to keep the framework's default order that prefers profile over keys if the profile was given as a CLI arg?

thomasmichaelwallace commented 12 months ago

Yeah, effectively this plugin exists because you can't just swap out the existing provider chain because the original serverless framework opted to use a custom order of preference.

One of the challenges of putting EC2MetadataCredentials in is that, because it calls a local server endpoint, it causes a delay while the connection waits to timeout if it is called where the endpoint is unreachable (i.e. a local machine).

tim-hutchinson-kc commented 12 months ago

I do have this written locally and it works in CI, since it can use the TokenFileWebIdentityCredentials, which is what Github/Gitlab/Bitbucket are preferring to leverage for workload identity.

Right now, I've got the order as the following which keeps the order currently in the plugin, then adds the remaining providers in their default order, but it's really up to you how you would want it to work, if you were to accept a PR.

[
  AWS.SharedIniFileCredentials,
  SsoCredentials,
  AWS.ECSCredentials,
  AWS.ProcessCredentials,
  AWS.TokenFileWebIdentityCredentials,
  AWS.EC2MetadataCredentials,
]

One of the challenges of putting EC2MetadataCredentials in is that, because it calls a local server endpoint, it causes a delay while the connection waits to timeout if it is called where the endpoint is unreachable (i.e. a local machine).

That's a good point about the remote endpoints causing delays, especially given that you can end up with relatively long chains if you have profile set in multiple places. We never run into that because we essentially only use the standard AWS_PROFILE variable, so we'll only hit one set of profile providers, and it's basically a (slightly out of order) replication of the standard AWS provider chain behavior.

Presumably it's also part of the rationale for why the AWS standard doesn't support multiple profiles, but rather separates a mechanism for selecting the profile and then applies that to the chain, so that you can only ever have one slow operation. The difference here is that right now the implementation in this plugin will try multiple profile options, instead of just picking the highest priority profile only.

thomasmichaelwallace commented 12 months ago

Yes, the complication is that serverless mixes the concepts of stages and profiles a bit when it comes to credential resolution. To be fair, it does pre-date the notion that you should separate your workloads by accounts đŸ˜„

I suspect your main motivation is to enjoy these fallbacks in CI/CD, so before you do any work, have you seen the approach to just disabling the plug-in in a CI/CD suggested here: https://github.com/thomasmichaelwallace/serverless-better-credentials/pull/33 ?

Realistically this plugin has a limited life, as serverless will eventually switch to aws-sdk v3 which will change the credential resolution process once again.

tim-hutchinson-kc commented 12 months ago

I've seen that, and it isn't something we would use in our company, because we explicitly want the plugin to handle credential resolution, just supporting more providers in the standard way that those using tools leveraging the AWS SDKs expect. We'd just use our fork, but would love if we could upstream the change so that you don't have to tell people to disable it.

I'd maybe also frame my questions this way: This plugin effectively replaces the framework's credentials resolution process. As such, you have full choice over which pieces of that process you keep vs replace. My questions are really just poking at the non-standard things I noticed, and figuring out if you want to keep them or change them. Both are totally valid answers with pros/cons

thomasmichaelwallace commented 11 months ago

OK - I'm releasing this with the PR as 2.0.

https://github.com/thomasmichaelwallace/serverless-better-credentials/releases/tag/v2.0.0

I don't really have much time to maintain this project so I haven't had a chance to see if it does cause timeout issues on people who are running locally (i.e. without local metadata services).