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

Handle API Key and API Secret properly #34

Closed
dkundel opened this issue Jul 13, 2019 · 4 comments
Closed

Handle API Key and API Secret properly #34

dkundel opened this issue Jul 13, 2019 · 4 comments
Assignees
Labels

Comments

@dkundel
Copy link
Member

dkundel commented Jul 13, 2019

Right now when a project is set-up using the Twilio CLI we are actually setting the API Key and API Secret that the Twilio CLI is using as ACCOUNT_SID and AUTH_TOKEN in the .env file. That works for deployment and interacting with the Twilio Serverless API because it doesn't require the knowledge of the Account SID.

However, it causes two problems.

  1. If you use context.getTwilioClient() to get a Twilio client this is going to fail for some APIs that require the knowledge of the Account SID (SMS, Calls etc.)
  2. We cannot verify locally the Twilio Signature for protected webhooks because we need the auth token for that which we don't have in that situation.

Why do we use the Auth Token in the first place?
In the legacy Functions environment we have ACCOUNT_SID and AUTH_TOKEN available on the context object. This enabled the use of context.getTwilioClient() but people could also access these variables directly. Therefore we are setting them upon project creation in your .env file.

By default these are also read for other interactions with the Serverless API for consistency. For deploy and other commands we introduced two flags --auth-token and --account-sid to override the ones that are read from the .env file.

When the Serverless Plugin was built, this introduced another complexity. The logic that is being used right now is the following:

  1. When serverless:init is called we'll pass username and password of the Twilio Client in the CLI to create-twilio-function. This will write them as ACCOUNT_SID and AUTH_TOKEN into .env.
  2. When you run serverless:start both ACCOUNT_SID and AUTH_TOKEN are being read and made available on context argument inside Function executions
  3. For deployment and other API interactions we follow the following logic:
    i) if a specific project has been passed with -p or --project we will use that username and password to interact with the API.
    ii) if the user passed in --account-sid and --auth-token we'll use those credentials
    iii) if the user as ACCOUNT_SID and AUTH_TOKEN in the .env file we'll use those
    iv) otherwise we'll fallback to the default credentials of the Twilio CLI that are under username and password.

Right now my train of thought here is:

  1. Rename --account-sid and --auth-token for API-related commands to --username and --password for consistency and deprecate the other ones
  2. Still keep ACCOUNT_SID and AUTH_TOKEN and set them to username and password essentially. Meaning these could be API Key and API Secret
  3. Detect if the pair is actually API Key and API Secret and display a warning to the user for the start command that client.getTwilioClient() and protected routes might not operate as expected and that if they want to make this more realistic, they have to change to Account SID and Auth Token.

Alternatively we can fix client.getTwilioClient() by detecting in create-twilio-function already that we are setting an API Secret not an Auth Token and instead set ACCOUNT_SID to the actual account SID and introduce TWILIO_API_KEY and TWILIO_API_SECRET instead and use that in creating the getTwilioClient() locally but then we will have the issue that context.AUTH_TOKEN will be available in the deployment but not locally. And since that's a sensitive piece of information, I wouldn't want to surprise the user that it exists.

Would love to have some input from you folks on how to solve this problem.
@philnash @dprothero

@ktalebian
Copy link
Collaborator

ktalebian commented Jul 13, 2019

I prefer if we encourage teas to use API Key and Secret. That's a better/safer way to be using credentials anyway since you can rotate them easily, and if you leak an API Key, you can just revoke it.

Why are you saying if you have APIKey/Secret things might not work normally? I would say the reverse, if they are using accountSid/authToken, we should warn them to use apiKey/Secret!

@philnash
Copy link
Contributor

I understand that we want to encourage use of API Key and Secret. But for Functions we also need:

  • Account SID for when we use context.getTwilioClient and need an account sid in the path
  • Auth Token so that we can validate incoming webhooks in a protected endpoint (which should be every webhook)

I'd be happy to add an optional prompt to ask for an API Key and Secret when initialising a project with create-twilio-function, but the most important things I feel that needs is the Account SID and Auth Token

Will new Functions continue to have ACCOUNT_SID and AUTH_TOKEN? And will there be any other blessed environment variables? Should there be some sort of consistency across the CLI, these projects and the Functions deploy environment.

@dkundel
Copy link
Member Author

dkundel commented Jul 15, 2019

Like @philnash said, some APIs require an Account SID in the API path. The Twilio CLI has that aspect so we could bridge that functionality.
However, the problem would remain that ACCOUNT_SID and AUTH_TOKEN will be available in Twilio Functions during runtime if credentials are enabled (which if they use this tool to deploy, they are automatically atm).
I think the fact that we wouldn't be able to verify the signature during local development would be a minor issue as this would be something that we could send a warning message about. Something like:

WARNING: You are currently using API Keys & Secret. While this is the preferred way, it currently does not allow you to verify webhook requests. Therefore local Functions won't be verifying the request signature. Learn more at: xxxx

@dkundel
Copy link
Member Author

dkundel commented Jul 26, 2019

This has been solved for now by using ACCOUNT_SID and AUTH_TOKEN as fields for either API Keys/Secret or Account SID / Auth Token. If in local development someone uses context.getTwilioClient() with an API Key/Secret pair, it will display an error as introduced in #45.

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

No branches or pull requests

3 participants