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

Certificates encrypted with static IV per key #117

Closed
rpicard opened this issue Oct 6, 2015 · 12 comments
Closed

Certificates encrypted with static IV per key #117

rpicard opened this issue Oct 6, 2015 · 12 comments

Comments

@rpicard
Copy link
Contributor

rpicard commented Oct 6, 2015

The IV is static per key at least. Lemur is using sqlalchemy_utils to encrypt certificates. This in turn
encrypts with AES in CBC mode.

https://github.com/kvesteri/sqlalchemy-utils/blob/master/sqlalchemy_utils/types/encrypted.py#L56

Given a single key, it will use the SHA256 hash of that key for all encryption. It looks like it will use the first 16 bytes of that hash as the IV for each operation.

@rpicard
Copy link
Contributor Author

rpicard commented Oct 6, 2015

Also reported upstream: kvesteri/sqlalchemy-utils#166

@rpicard rpicard changed the title Certificates encrypted with static IV Certificates encrypted with static IV per key Oct 6, 2015
@rpicard
Copy link
Contributor Author

rpicard commented Oct 7, 2015

I think a reasonable fix would be either using the FernetEngine with sqlalchemy-utils or removing the middle-man and using the cryptography package directly. Using Fernet, with or without sqlalchemy-utils, also has the benefit of adding an HMAC.

@rpicard rpicard mentioned this issue Oct 7, 2015
@kevgliss
Copy link
Contributor

kevgliss commented Oct 7, 2015

Makes sense, is simply passing engine=fernet to EncryptedType enough? Will need to think about migrating existing key pairs to the new encryption scheme.

Ditching sqlalchemy_utils may be a good idea, I have to see if I use it for anything else. Do you want to take a stab at creating a TypeDecorator?

If we do end up re-writing it, MultiFernet looks like it might make a nice addition, especially if someone has a requirement to rotate keys periodically.

@rpicard
Copy link
Contributor Author

rpicard commented Oct 7, 2015

Yeah, I'll work on making a MultiFernet TypeDecorator over the next couple of days.

@rpicard
Copy link
Contributor Author

rpicard commented Oct 9, 2015

We have a few options for taking the user's key and passing it to Fernet:

  1. Require that the user's keys are 32 bytes, base64 encoded. In the documentation we can recommend doing Fernet.generate_key() or something similarly secure. We could even add a command to the CLI to insert a random key into the config.
  2. Run the user's key through HKDF to expand it to the right size.
  3. Run the user's key through PBKDF to expand it to the right size, plus add some protection against brute forcing when they have a password instead of a securely random and long key. This would require a salt, which brings up the question of where to store it. We could either store it in the database somewhere, have the user put it in their configuration, or use a static salt. This would prevent rainbow tables from helping with the brute forcing.

I prefer the first option, but it does sacrifice some convenience, so I don't want to make assumptions about your priorities.

@kevgliss Do you have any thoughts?

@kevgliss
Copy link
Contributor

kevgliss commented Oct 9, 2015

@rpicard I prefer the first option as well, this this is the approach I take when generating configuration files for the user.

https://github.com/Netflix/lemur/blob/master/lemur/manage.py#L167

And also for the "lock" and "unlock" functions that is also using fernet.

https://github.com/Netflix/lemur/blob/master/lemur/manage.py#L419

@rpicard
Copy link
Contributor Author

rpicard commented Oct 9, 2015

Perfect. 💯

@rpicard
Copy link
Contributor Author

rpicard commented Oct 9, 2015

Is this try / except to catch the "working outside of application context" error? I don't fully understand what is causing that error. Is it just that there isn't a current_app when running something like lemur init?

https://github.com/Netflix/lemur/blob/master/lemur/utils.py#L19

@kevgliss
Copy link
Contributor

kevgliss commented Oct 9, 2015

Correct, it is an unfortunate hack, flask-script needs a flask app object. I use flask-script to run create_config that generates configurations for a user... because they don't have one yet, but the sqlalchemy_utils type-decorator needs something. This was my way of breaking that circular dependency.

Ideally flask-script wouldn't need current_app to function. I looked at http://click.pocoo.org/5/ which doesn't need an app object, but I wasn't sold on the decorator soup it tends to lean on.

@rpicard
Copy link
Contributor Author

rpicard commented Oct 9, 2015

Okay, that explains the problem I had. Like you said, it's an unfortunate hack, but I can live with it.

rpicard added a commit to rpicard/lemur that referenced this issue Oct 10, 2015
Facilitates key rotation and uses more secure encryption than what
sqlalchemy-utils does.

Fixes Netflix#117 and Netflix#119.
@rpicard
Copy link
Contributor Author

rpicard commented Oct 12, 2015

I've been running in Docker, but I'm trying to debug some things and to do so, I want to just run on my machine normally. I'm now getting this error when I run lemur init. The database is being created just fine.

https://gist.github.com/rpicard/db776eebc25742460eff

@rpicard
Copy link
Contributor Author

rpicard commented Oct 12, 2015

My bad. I missed the lemur db init step.

rpicard added a commit to rpicard/lemur that referenced this issue Oct 13, 2015
Facilitates key rotation and uses more secure encryption than what
sqlalchemy-utils does.

Fixes Netflix#117 and Netflix#119.
nezdolik pushed a commit to spotify/lemur that referenced this issue Mar 23, 2023
Update dev docs and fixing mac and linux differences
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

No branches or pull requests

2 participants