Skip to content

Streamlining the credentials confusion #163

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

Closed
dkundel opened this issue May 16, 2020 · 7 comments
Closed

Streamlining the credentials confusion #163

dkundel opened this issue May 16, 2020 · 7 comments
Assignees
Milestone

Comments

@dkundel
Copy link
Contributor

dkundel commented May 16, 2020

Background

Twilio Functions provides you in the deployed environment access to ACCOUNT_SID and AUTH_TOKEN both as part of the context as well as exposed in process.env. When twilio-run was initially created for local development we opted to mirror this as best as possible. When create-twilio-function came along we stuck with the same system.

However, when Twilio CLI was created they explicitly went with API Key and API Secret instead of Account SID and Auth Token. Since the Serverless API doesn't really care about which of these two pairs it gets, we did a "quick fix" of setting the API Key and API Secret as Account SID and Auth Token respectively.

The Issues

This "quick fix" results in you being able to run npm run deploy without a problem, however, causes several problems.

  1. Confusion. People look at ACCOUNT_SID inside their .env file and are surprised when it's a API_KEY.
  2. Bad experience when using context.getTwilioClient(). In order to mirror the behavior of Twilio Functions getTwilioClient() uses the values from ACCOUNT_SID and AUTH_TOKEN but also requires that those are actually Account SID and Auth Token. If that's not the case the user gets an error and has to manually change those.

Potential Solutions

There's two solutions to this dilemma. Regardless of the two cases the following should be a given:

Both options will require a major version bump.

Option 1

  • When you create a project with create-twilio-function we will continue to ask for Account SID and Auth Token and will populate those in the .env file for the user
  • When you create a project with twilio serverless:init we will only populate ACCOUNT_SID in the .env file
  • When a user runs twilio-run deploy we will use the credentials from the .env file unless something else is specified via CLI arguments
  • When a user runs twilio serverless:deploy we will use the credentials stored in the Twilio CLI, disregarding the values in .env [BREAKING CHANGE] this is a breaking change because currently even this command would prefer the .env values unless a profile is explicitly passed via twilio serverless:deploy -p default

Aside of that last change there shouldn't be much of an impact for users in this case. They will still have to add the Auth Token manually though unless getting it magically to work if they want to use context.getTwilioClient() but at least the error will be more useful.

Option 2

  • When you create a project with create-twilio-function we will continue to ask for Account SID and Auth Token and will populate those in the .env file for the user
  • When you create a project with twilio serverless:init we will populate ACCOUNT_SID as well as create a TWILIO_API_USERNAME and TWILIO_API_PASSWORD variables in the .env file
  • Disregarding which version of the command you use to deploy we will still prefer the credentials in the .env file.
  • We will have to block TWILIO_API_USERNAME and TWILIO_API_PASSWORD from being uploaded when uploading other .env variables.
@dkundel
Copy link
Contributor Author

dkundel commented May 16, 2020

@philnash let me know what you think when you have some time

@philnash
Copy link
Contributor

Option 1 sounds right to me, when you use the CLI you expect it to work the same across plugins, and that includes using your default profile credentials.

Note: create-twilio-function can, and will, ask for only the auth token if the account sid is already set. It can be skipped too, but for the best experience when running locally, I'd want to try to get it set in the .env regardless of how the project was initiated.

@dkundel dkundel transferred this issue from twilio-labs/serverless-toolkit-moved Aug 8, 2020
@NReilingh
Copy link
Contributor

Wanted to add a highly related issue that I was experiencing yesterday. I wasn't using getTwilioClient(), but I was using Twilio.jwt.AccessToken's constructor, which is passed ACCOUNT_SID, API_KEY, and API_SECRET. (Project was based on the voice-client-javascript function template.)

Wanting to run this from a local dev server, I added the three variables to .env, but then my deployments started failing -- it was because ACCOUNT_SID in .env was overriding the API Key stored for the CLI, and then being used with the CLI's API Secret. I had to add -p <my profile name> to my command, which seems silly since it's already supposed to be my active profile.

I think I prefer option 1 of the two that you propose. +1 about using the default profile credentials when they are available. It feels wrong to me, generally speaking, to have the .env file affect the deployment at all. My mental model is that .env handles the runtime's internal environment; whatever credentials used to deploy to that runtime must be managed outside of .env. For example, if I wanted to run my deployments from a CI pipeline, these credentials would be passed in using the pipeline's environment. If I wanted to abstract even this locally, I would use a tool like direnv to set shell environment variables based on my working directory.

To elaborate on this a little bit more, there might be something to learn from the gcloud CLI -- you can switch between profiles using a shell environment variable. Direnv makes this very easy to accomplish. Currently with the twilio CLI, you'd have to use the direnv hook to call twilio profiles:use instead of just setting a shell environment variable -- still kind of possible, but much messier.

I think it's probably fine for a .env file to contain deployment credentials when they are the same as what the runtime would actually provide via context or process.env. But I think it needs to always be possible to provide deployment credentials outside of .env, likely via shell environment variable (TWILIO_API_KEY, TWILIO_API_SECRET), CLI profile (controllable via shell environment variable), and CLI options.

Also not that you should care what I think 😉, but I prefer API Key/API Secret to username/password. I think it's better to try to teach people that their ACCOUNT_SID and AUTH_TOKEN can also be used as an API Key and Secret than it is to teach people that username/password DON'T mean the username and password that they already have for Twilio itself, but instead mean either one of two different types of credential with different names.

@dkundel
Copy link
Contributor Author

dkundel commented Aug 10, 2020

Thank you @NReilingh for your feedback. We always love getting feedback from customers and the community and I'm sorry you hit this roadblock.

The behavior should definitely be fixed with Option 1. I think improving the way the configuration works (#166) should help with that as well.

In terms of CI and pulling in variables from your system, we recently fixed that in #144 where you can use the --load-system-env flag. The only thing we did to make it more compatible with CIs was to use .env files as template and only fallback to the system env for empty variables. The reason for that is to avoid for example in GitHub Actions to upload all the various environment variables that are set as part of the CI.

That in combination with #166 should enable you to define a config such as:

{
  "loadSystemEnv": true,
  "env": ".env.sample"
}

That will enable you to use direnv and load the environment variables from there. Cosmiconfig (proposed in #166) would even allow you to put this on your user directory not having to worry about setting those config values on every project.

@dkundel
Copy link
Contributor Author

dkundel commented Aug 10, 2020

Also as per your last point: I generally agree on educating people that API Key and Secret can work like Account SID and Auth Token.

I think for the command line arguments we will transition to --username and --password though and add a description to make it clear in order to avoid having: --account-sid , --auth-token, --api-key and --api-secret.

@philnash
Copy link
Contributor

As part of Option 1, we should enhance the getTwilioClient function to check for an auth token as well as an Account Sid.

philnash added a commit that referenced this issue Mar 5, 2021
As described in #163, never use .env variables to set account sid and auth token when called from the Twilio CLI.

BREAKING CHANGE: deploys may not work as previously
philnash added a commit that referenced this issue Apr 16, 2021
As described in #163, never use .env variables to set account sid and auth token when called from the Twilio CLI.

BREAKING CHANGE: deploys may not work as previously
philnash added a commit that referenced this issue Apr 19, 2021
As described in #163, never use .env variables to set account sid and auth token when called from the Twilio CLI.

BREAKING CHANGE: deploys may not work as previously
@dkundel
Copy link
Contributor Author

dkundel commented Apr 20, 2021

Fixed in #224

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants