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

Adds configuration for server-side encrypted s3 uploads #161

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

rauhryan
Copy link
Member

@rauhryan rauhryan commented Jan 9, 2016

No description provided.

@rauhryan rauhryan self-assigned this Jan 9, 2016
@rauhryan rauhryan changed the title [WIP] Adds configuration for server-side encrypted s3 uploads Adds configuration for server-side encrypted s3 uploads Jan 17, 2016
@rauhryan
Copy link
Member Author

@huboard/committers I need to cut an enterprise release for this and I don't have time to wait for the official process.

  1. merge master into enterprise
  2. merge this PR into enterprise
  3. cut new release
  4. wait for review and merge this PR into master
  5. ???? figure out how to fix the enterprise branch (help me @dahlbyk?)

I've taken WIP off, so if enough people are online to not make this happen that would be better

@dahlbyk
Copy link
Member

dahlbyk commented Jan 19, 2016

Just to confirm, this PR adds four environment variables:

  • AWS_ENABLED feature toggle
  • AWS_S3_ENDPOINT to override AWS URL (default: https://s3-us-west-2.amazonaws.com)
  • AWS_S3_ENCRYPTED to enable encryption given true
  • AWS_KMS_KEY_ID to specify that AWS should use an encryption key other than the default (docs)

It also adds an ENCRYPTED_UPLOADS feature toggle.

Really my only question is why we need both ENCRYPTED_UPLOADS and AWS_S3_ENCRYPTED - it seems we shouldn't need to require both.


if(HUBOARD_ENV.FEATURES.ENCRYPTED_UPLOADS) {
Copy link
Member

Choose a reason for hiding this comment

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

As more generally commented, it seems to me that this could be:

if(HUBOARD_ENV.CONFIG && HUBOARD_ENV.CONFIG.AWS_S3_ENCRYPTED === 'true')
...

@dahlbyk
Copy link
Member

dahlbyk commented Jan 19, 2016

Really my only question is why we need both ENCRYPTED_UPLOADS and AWS_S3_ENCRYPTED - it seems we shouldn't need to require both.

I see now that these are being populated from the same enterprise maintenance value behind the scenes. The IMAGE_UPLOADS feature is similarly aligned with AWS_ENABLED. We can :shipit: as-is, but we should revisit to settle on a single source of truth between client- and server-side.

@rauhryan
Copy link
Member Author

¯_(ツ)_/¯ I wish we had more time to fix this “right"

@dahlbyk
Copy link
Member

dahlbyk commented Jan 21, 2016

I wish we had more time to fix this “right"

We do before it hits master. So... do we need HUBOARD_ENV.FEATURES at all if we have HUBOARD_ENV.CONFIG?

@dahlbyk dahlbyk added this to the Engineering milestone Apr 15, 2016
@dahlbyk dahlbyk force-pushed the rauhryan/enterprise-image-upload branch 2 times, most recently from 4df21b1 to 6f835b4 Compare May 7, 2016 01:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants