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

Add Cloud SQL MySQL connectivity samples for SQLAlchemy. #1828

Merged
merged 2 commits into from
Nov 13, 2018

Conversation

kurtisvg
Copy link
Contributor

@kurtisvg kurtisvg commented Nov 8, 2018

New sample app in a series of samples designed to highlight connection best practices. The app is a simple web app that allows voting between two different groups ("Tabs" vs "Spaces") and records the votes in a MySQL backend.

Java equivalent is here.

@googlebot googlebot added the cla: yes This human has signed the Contributor License Agreement. label Nov 8, 2018
Copy link
Contributor

@tswast tswast left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Very useful sample, I like it. Just a comment regarding where to put the warning about environment variables. It's more that it's not secure to write them unencrypted and then check it into version control. Environment variables are a common way to pass in secrets to applications if they can be set securely.

export DB_PASS='my-db-pass'
export DB_NAME='my_db'
```
Note: Saving credentials in environment variables is convenient, but not secure - consider a more
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

12 factor app might disagree with you for this. https://12factor.net/config

It all depends on how you set the environment variables. Even Kubernetes secrets can be used to set environment variables, and I wouldn't call that "not secure".

Plus, KMS can't help you store your secrets, only encrypt/decrypt them.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have to disagree about the security of store secrets in an environment variable. Environment variables are accesible by any process that runs in the user's environment, leaving them vulnerable. Any secret of significant value should not be stored as an environment variable.

While KMS doesn't directly store secrets, they do have a page on how to store secrets using KMS: https://cloud.google.com/kms/docs/store-secrets

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've update the wording to condone more specifically against condoning plain-text secrets and added it to the app.yaml as well.


## Google App Engine Standard

To run on GAE-Standard, create an AppEngine project by following the setup for these
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

space between App Engine


## Running locally

To run this application locally, download and install the cloud_sql_proxy by
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

backticks around cloud_sql_proxy or use the proper name (Cloud SQL proxy)

import sqlalchemy


# Saving credentials in environment variables is convent, but not secure
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ditto. This isn't the worst practice, and might actually offend the 12-factor app developer community. This comment is appropriate in the app.yaml file, though. You don't want people checking secrets into version control.

env_variables:
CLOUD_SQL_INSTANCE_NAME: <MY-PROJECT>:<INSTANCE-REGION>:<MY-DATABASE>
DB_USER: my-db-user
DB_PASS: my-db-pass
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Add a comment that storing passwords in version control is not secure. Suggesting you encrypt the password with KMS would be appropriate here.

@kurtisvg kurtisvg merged commit 266b254 into master Nov 13, 2018
@kurtisvg kurtisvg deleted the cloudsql-connect branch November 13, 2018 03:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cla: yes This human has signed the Contributor License Agreement.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants