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

Added pgp_key property for password encryption #24

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

Conversation

phajduk
Copy link
Contributor

@phajduk phajduk commented Dec 20, 2018

No description provided.

@phajduk
Copy link
Contributor Author

phajduk commented Dec 20, 2018

@frankfarrell I implemented new feature which is based on official AWS provider and IAM users. This change allow to generate and encrypt password using PGP Keybase.

@frankfarrell
Copy link
Owner

frankfarrell commented Dec 20, 2018

@phajduk Thanks for the PR.

Its an interesting feature. Just a few comments,

  1. I'm a bit ambivalent about adding support for something in this provider that isn't supported by the underlying API as such.
  2. I haven't read your password generation code closely, but it seems a bit too much responsibility to have this provider generating passwords on the fly. What if there's a bug? Who is liable (morally if not legally)?
  3. There's a lot of whitespace added in the PR, it makes it a little hard to seen to signal for the noise.

Perhaps, allowing users to specify PGP Key and the encrypted password would be a good compromise? I considered this for kms encrypted password, but that would take more aws configuration so I'm not sure its a good idea either

@phajduk
Copy link
Contributor Author

phajduk commented Dec 20, 2018

Hi @frankfarrell

  1. This is supported by Terraform out of the box. By this I mean support for PGP encryption using Keybase public keys. I've added mentions about this fact in few places (in readme and in the code).
  2. Those passwords are random chars with fixed length. We can randomize length above some level as well. Only the user who's PGP was used to encrypt pass will be able to decrypt password. In order to login, users password has to be decrypted so user will get a chance to view&use the pass in plaintext. It's still users responsibility to change it if it's not safe for him.
  3. I will revert those whitespace fixes if you want. Auto formatter removed redundant spaces at the end of few lines in README.

Regarding proposition about providing encrypted password. It may work as well of course. However I can't see any reason to not support both cases:

  • if pgp_key is provided and there's encrypted_password provided then decrypt password and use it
  • if only pgp_key is provided then generate some password

@frankfarrell
Copy link
Owner

Sorry, I should have been clearer on point 1, I meant that the underlying redshift DDL doesn't support the PGP encryption out of the box.
Agreed about the ultimate responsibility of user's security resting with the user, but I'm just worried about cases where a password look random but is somehow predictable to some malelovent actor. Maybe paranoid, but in general I think its a good idea to limit the scope of any software.
Thanks for the fix.

I don't mean to dismiss this stuff, its a really good PR so thanks fo rthat. Its just a big change so trying to think critically about it!

@phajduk phajduk force-pushed the pgp_key_property branch 4 times, most recently from 5ce2861 to efedf7e Compare December 20, 2018 14:06
@phajduk
Copy link
Contributor Author

phajduk commented Dec 20, 2018

@frankfarrell I've checked encryption implementation and they didn't support decryption in same way as they support encryption. We would need to write much more code and use vault helper.
In general I agree with your paranoid-safety-first approach. However, I still can see use cases where we need to manage multiple users and Redshift is secured other ways e.g. network layer. Also this solution is based on HashiCorp bits and it's battle tested. I don't have more arguments as basically I agree with you it may be insecure in some cases.

@phajduk
Copy link
Contributor Author

phajduk commented Jan 2, 2019

@frankfarrell are you interested in merging those changes? Not sure if I should implement anything more here or should I maintain it in separated fork.

@ryan-carlson
Copy link
Contributor

I am also interested in some password encryption feature. I was envisioning a similar but slightly different solution like this:

  • Encryption key is provided as a configuration to the provider. This can be done using a Terraform variable or in our case pulled from AWS Secrets manager.
  • "redshift_user" resource passwords are encrypted (manually) when configuring the resource
  • Encrypted version of the password is stored in Terraform state. This has the benefit that the password is printed in encrypted format when Terraform compares the state.
  • Password is decrypted only when calling "create user" or "alter user" against Redshift.

Let me know your thoughts. We are still managing user creation outside of Terraform for this reason, but it would be great to have some support for encryption. I am open to collaborating on this too if the approach sounds reasonable.

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

Successfully merging this pull request may close these issues.

3 participants