Open rpicard opened 9 years ago
From what I understand we do it this way so that encrypt("b") == encrypt("b")
to allow for queryability of encrypted values. There is the https://github.com/kvesteri/sqlalchemy-utils/blob/master/sqlalchemy_utils/types/encrypted.py#L94 which uses (from the documentation):
If you're suggesting that the Fernet engine be the default I'd honestly be fine with that as long as we push it out as a minor version bump.
Making the Fernet engine the default sounds good to me. Getting the HMAC for free is a win too.
With that said, if AesEngine
is kept around it should still be fixed.
With that said, if AesEngine is kept around it should still be fixed.
Except it's done that way to allow querying. Depending on the why of your encryption it may be fine.
Using a static IV per key compromises security. If users are willing to accept that to gain the convenience it provides, then that's up to them. If you want to facilitate that it's up to you. If that's your decision, my opinion is that the documentation should make it clear beyond any question that using those features reduces the security of the system.
That's my point of view. Remediate this as you see fit. :+1:
I'm going to recommend (and then probably not get time to do it for a while so if anyone wants to jump in it'd be appreciated):
Fernet
enginereproducible
or queryable
) that switches to the as-standing AesEngine
I agree that leaking a database encrypted with static IVs would allow a line of reasoning about the encrypted data that otherwise wouldn't exist (for instance, if we were talking about salaries of employees you could see that 2 people make the same amount). But I don't agree that its broken. Security is a matter of degree. Normally only data that should be encrypted gets encrypted. However if your client instructs you to encrypt the world, no matter what, then allowing some data to be queryable is a thing that would need to happen.
@mehcode , @rpicard hello and i am very sorry i am responding so late. First i would like to say that all of your above comments are absolutely correct. To be honest when i started the EncryptedType
i wanted to give users the possibility to also query values in the DB and that is why the IV is not randomly generated. It was just a decision, maybe a wrong one... When i have the time i will try to make the changes proposed by @mehcode in the above comment.
So it's been a little over 5 years since this ticket was opened, so here's a summary of updates that I've noticed when looking at this.
This ticket was opened when the latest release was 0.31.0, and we're now on 0.36.8.
The file https://github.com/kvesteri/sqlalchemy-utils/blob/0.31.0/sqlalchemy_utils/types/encrypted.py has been replaced by the directory https://github.com/kvesteri/sqlalchemy-utils/tree/master/sqlalchemy_utils/types/encrypted/
The classes AesEngine
and FernetEngine
still exist, and there is a new AesGcmEngine
.
AesEngine
and AesGcmEngine
have docstrings explaining that the former allows searching by value of an encrypted column, but is less secure than the latter, which does not allow this searching. The Fernet
docstring does not mention that it does not enable this searching.
In the SQLAlchemy-Utils docs, the only mention of encryption is in https://sqlalchemy-utils.readthedocs.io/en/latest/data_types.html#module-sqlalchemy_utils.types.encrypted.encrypted_type which does not have a docstring so the user will need to look at the source code without prompting to see the docstrings of the engines and to see that AesEngine
is the default. It used to have a docstring but it now inherits from StringEncryptedType
and the docstring has been moved there. StringEncryptedType
does not appear in the documentation apart from in the source code. It looks like the docstring wasn't updated though when it was moved from StringEncryptedType
to EncryptedType
, so it is still using EncryptedType
in it's example code rather than StringEncryptedType
.
The changes proposed by @mehcode still look appropriate to me, with the addition that the documentation should make clear that EncryptedType
inherits from StringEncryptedType
and the docstring of StringEncryptedType
should be available in the documentation with some notes about the engines.
Neither databases or encryption are my domain, but this seems fairly simple so I'll have a go at it this weekend.
Is anyone aware of any part of https://github.com/sqlalchemy/sqlalchemy or related projects that could use documentation updates based on this?
Pull request opened. #499
I ended up keeping the engine
parameter so that when queryable
is False
the user has the choice of AesGcmEngine
and FernetEngine
with the latter being the default. If engine
is specified it must be compatible with the value of queryable
.
Looks like the move was called out above, but here are permalinks...
The original link:
https://github.com/kvesteri/sqlalchemy-utils/blob/4a9b7c7df67098cd69c74e0e54cc7889d5a27465/sqlalchemy_utils/types/encrypted.py#L56
The current location of the same code:
https://github.com/kvesteri/sqlalchemy-utils/blob/5c4ca8ca98aba2fcbddd0d9ad455668286d94f8d/sqlalchemy_utils/types/encrypted/encrypted_type.py#L82
pipenv check
trips on this: https://pyup.io/v/42194/742/
Anyone know of a suitable replacement until this is fixed?
EncryptedType uses AES in CBC mode. The IV that it uses is not random though.
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.
This link is a good primer on why this is bad: http://security.stackexchange.com/a/1097