ADD crypto-fields #160

kamil1marczak · 2021-04-24T21:54:56Z

Hi I have added package of encrypted fields,

codecov-commenter · 2021-04-24T21:58:10Z

Codecov Report

Merging #160 (714ecae) into master (252e681) will increase coverage by 0.01%.
The diff coverage is 93.16%.

@@            Coverage Diff             @@
##           master     #160      +/-   ##
==========================================
+ Coverage   93.02%   93.04%   +0.01%     
==========================================
  Files           9       11       +2     
  Lines         789      949     +160     
==========================================
+ Hits          734      883     +149     
- Misses         55       66      +11

Impacted Files	Coverage Δ
drf_extra_fields/crypto_fields.py	`88.40% <88.40%> (ø)`
tests/test_crypto_fields.py	`96.70% <96.70%> (ø)`
drf_extra_fields/runtests/settings.py	`86.66% <100.00%> (ø)`

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 252e681...714ecae. Read the comment docs.

yigitguler · 2021-05-10T09:03:06Z

Hi @kamil1marczak, thank you for this detailed PR.

We will review it and get back to you.

alicertel · 2021-05-11T12:39:02Z

drf_extra_fields/crypto_field.py

+    Cryptography protocol used in mixin: Fernet (symmetric encryption) provided by Cryptography (pyca/cryptography)
+    """
+
+    def __init__(self, salt_settings_env=None, password=None, *args, **kwargs):


salt_settings_env sounds like an environment variable but it's not actually. Why not this is directly provided like password? Why there is an inconsistency?

I suggest to have DEFAULT_CRYPTOGRAPHY_SALT and DEFAULT_CRYPTOGRAPHY_PASSWORD variables in settings.py. These must be defined to be able to use crypto fields. If salt or password is not provided along with field definition these will be used. What do you think?

in an updated version, I use Django SECRET_KEY as salt but I kept the password as an optional parameter to make it more dynamic (the whole idea is to have something static - salt and dynamic - password as encryption ingredients)

omerfarukabaci

Thank you for your PR, we much appreciate it! However, I guess there is a misunderstanding here (and yes, I have realized it now, after reviewing your PR... 😓). This package includes extra fields for DRF (Django Rest Framework) Serializer fields, not for Django model fields (at least until now). I don't know what can/should we do about it. Any thoughts @alicertel?

omerfarukabaci · 2021-05-11T13:44:38Z

README.md

+## CryptoField Package
+A set of generic common Djano Fields that automatically encrypt data for database amd encrypt for the purpose of usage in Django.
+* *CryptoFieldMixin* - Mixin that implement encrypt/decrypt methods.
+    Example of how to use to create custom CryptoTextField


Suggested change

## CryptoField Package

A set of generic common Djano Fields that automatically encrypt data for database amd encrypt for the purpose of usage in Django.

* *CryptoFieldMixin* - Mixin that implement encrypt/decrypt methods.

Example of how to use to create custom CryptoTextField

## CryptoField Package

A set of generic common Django Fields that automatically encrypts data for the database and for the purpose of usage in Django.

* *CryptoFieldMixin* - A mixin that implements encrypt/decrypt methods.

Example of how to use it to create the CustomCryptoTextField

omerfarukabaci · 2021-05-11T13:56:18Z

README.md

+**Settings.**
+each CryptoField has 2 kwargs *salt_settings_env* and *password*.
+* *salt_settings_env* - name of variable stored in settings.py file, which will be used as cryptographic salt. default: salt_settings_env = 'SECRET_KEY' if settings not set or no SECRET_KEY in setting default: salt = "Salt123!!!"
+* *password* - password to be used in encryption process of given field (together with salt set globally) default = 'password'


Suggested change

**Settings.**

each CryptoField has 2 kwargs *salt_settings_env* and *password*.

* *salt_settings_env* - name of variable stored in settings.py file, which will be used as cryptographic salt. default: salt_settings_env = 'SECRET_KEY' if settings not set or no SECRET_KEY in setting default: salt = "Salt123!!!"

* *password* - password to be used in encryption process of given field (together with salt set globally) default = 'password'

### Settings

Each CryptoField has 2 kwargs: `salt_settings_env` and `password`.

* `salt_settings_env`: Name of the variable stored in the settings.py file, which will be used as cryptographic salt.

**Default**: `"SECRET_KEY"`

**Note**: If `salt_settings_env` does not exist in `settings.py` then `salt` will be `Salt123!!!`

* `password`: Password to be used in the encryption process of the given field (together with salt set globally).

**Default**: `"password"`

omerfarukabaci · 2021-05-11T14:00:34Z

README.md

+**Example.**
+
+```python
+    # models.py
+
+    from django.db import models
+
+    from drf_extra_fields.crypto_field import *
+
+    class TestCryptoEmail(models.Model):
+
+            value = CryptoEmailField(salt_settings_env='NEW_SECRET_KEY', password='new_password')


Suggested change

**Example.**

```python

# models.py

from django.db import models

from drf_extra_fields.crypto_field import *

class TestCryptoEmail(models.Model):

value = CryptoEmailField(salt_settings_env='NEW_SECRET_KEY', password='new_password')

## Example

```python

# models.py

from django.db import models

from drf_extra_fields.crypto_field import CryptoEmailField

class User(models.Model):

encrypted_email = CryptoEmailField(salt_settings_env='NEW_SECRET_KEY', password='new_password')

omerfarukabaci · 2021-05-11T14:51:19Z

drf_extra_fields/crypto_field.py

+def to_bytes(_obj):
+    if isinstance(_obj, bytes):
+        return _obj
+    elif isinstance(_obj, (bytearray, memoryview)):
+        return force_bytes(_obj)
+    else:
+        return pickle.dumps(_obj)


This method doesn't work as expected for bytearray:

to_bytes(bytearray([1, 2, 3, 4, 5])) # b"bytearray(b'\\x01\\x02\\x03\\x04\\x05')"

because force_bytes casts bytearray to str and then convert it to bytes. Here we don't need to use force_bytes, we can cast the object to bytes directly for bytearray and memoryview.

What about calling this method _to_bytes(), since this is a specific method for crypto_field?

What is the purpose of the underscore in _obj here? I guess using _to_bytes(obj) is fine.

In the new version, I use validators rather than force casting types

omerfarukabaci · 2021-05-11T14:53:13Z

drf_extra_fields/crypto_field.py

+try:
+    from functools import cached_property as property_decorator
+except ImportError:
+    from builtins import property as property_decorator
+import base64
+import pickle
+from django.utils.translation import gettext_lazy as _
+from cryptography.fernet import Fernet
+from cryptography.hazmat.primitives import hashes
+from cryptography.hazmat.primitives.kdf.pbkdf2 import PBKDF2HMAC
+from django.conf import settings
+from django.core.checks import Error
+from django.core.exceptions import ImproperlyConfigured
+from django.db import models
+from django.utils.encoding import force_bytes


Suggested change

try:

from functools import cached_property as property_decorator

except ImportError:

from builtins import property as property_decorator

import base64

import pickle

from django.utils.translation import gettext_lazy as _

from cryptography.fernet import Fernet

from cryptography.hazmat.primitives import hashes

from cryptography.hazmat.primitives.kdf.pbkdf2 import PBKDF2HMAC

from django.conf import settings

from django.core.checks import Error

from django.core.exceptions import ImproperlyConfigured

from django.db import models

from django.utils.encoding import force_bytes

try:

from functools import cached_property as property_decorator

except ImportError:

from builtins import property as property_decorator

import base64

import pickle

from cryptography.fernet import Fernet

from cryptography.hazmat.primitives import hashes

from cryptography.hazmat.primitives.kdf.pbkdf2 import PBKDF2HMAC

from django.conf import settings

from django.core.checks import Error

from django.core.exceptions import ImproperlyConfigured

from django.db import models

from django.utils.encoding import force_bytes

from django.utils.translation import gettext_lazy as _

omerfarukabaci · 2021-05-11T15:40:18Z

drf_extra_fields/crypto_field.py

+    def get_db_prep_save(self, value, connection, prepared=False):
+        if self.empty_strings_allowed and value == bytes():
+            value = ""
+        value = super().get_db_prep_value(value, connection, prepared=False)
+        if value is not None:
+            encrypted_value = self.encrypt(value)
+            return encrypted_value
+            # return connection.Database.Binary(encrypted_value)


What about something like that?

Suggested change

def get_db_prep_save(self, value, connection, prepared=False):

if self.empty_strings_allowed and value == bytes():

value = ""

value = super().get_db_prep_value(value, connection, prepared=False)

if value is not None:

encrypted_value = self.encrypt(value)

return encrypted_value

# return connection.Database.Binary(encrypted_value)

def get_db_prep_save(self, value, connection, prepared=False):

if value:

value = self.encrypt(value)

return super().get_db_prep_value(value, connection, prepared)

omerfarukabaci · 2021-05-11T15:46:48Z

requirements.txt

@@ -1,2 +1,3 @@
 Django >= 2.2
 djangorestframework >= 3.9.2
+cryptography


Could you please pin the version?

version 3.2.1 is the last with python 3.5 support therefore I have pined that one

omerfarukabaci · 2021-05-11T15:50:22Z

tests/test_crypto.py

+)
+
+
+class PersonTest(TestCase):


Suggested change

class PersonTest(TestCase):

class CryptoFieldsTests(TestCase):

Also what about creating a model consisting of all crypto fields and making some insert & update & delete operations to make sure that the whole integration is working?

I have tested in that matter in updated version

omerfarukabaci · 2021-05-11T15:50:42Z

tests/test_crypto.py

@@ -0,0 +1,100 @@
+import datetime


Could you please rename this file to test_crypto_fields.py?

omerfarukabaci · 2021-05-11T15:51:51Z

drf_extra_fields/crypto_field.py

@@ -0,0 +1,211 @@
+try:


Could you please rename this file to crypto_fields.py? It makes more sense while importing:

from drf_extra_fields.crypto_fields import CryptoTextField

alicertel · 2021-05-11T16:15:14Z

Thank you for your PR, we much appreciate it! However, I guess there is a misunderstanding here (and yes, I have realized it now, after reviewing your PR... 😓). This package includes extra fields for DRF (Django Rest Framework) Serializer fields, not for Django model fields (at least until now). I don't know what can/should we do about it. Any thoughts @alicertel?

Good catch @omerfarukabaci . I didn't realize that. You are right our package includes only DRF fields. I believe this pr is more suitable to https://django-extensions.readthedocs.io/en/latest/field_extensions.html

kamil1marczak · 2021-05-12T16:07:31Z

I am really sorry for the obvious mistake, I will rewrite it to operate as serialization and include all of your comments from the review. I have few ideas in that matter Can I write you directly in case of seeking consultation on how to get the most utility out of it? Kamil wt., 11 maj 2021 o 18:15 alicertel ***@***.***> napisał(a):

…

Thank you for your PR, we much appreciate it! However, I guess there is a misunderstanding here (and yes, I have realized it now, after reviewing your PR... 😓). This package includes extra fields for DRF (Django Rest Framework) Serializer fields, not for Django model fields (at least until now). I don't know what can/should we do about it. Any thoughts @alicertel <https://github.com/alicertel>? Good catch @omerfarukabaci <https://github.com/omerfarukabaci> . I didn't realize that. You are right our package includes only DRF fields. I believe this pr is more suitable to https://django-extensions.readthedocs.io/en/latest/field_extensions.html — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#160 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/ANBYAGQUJPPUJFYADSVB7LDTNFJ2TANCNFSM43QRZHIA> .

kamil1marczak · 2021-05-13T05:48:49Z

@omerfarukabaci @alicertel everything has been rewritten to drf fields. I will be grateful for another review

alicertel · 2021-05-18T09:36:46Z

Thanks @kamil1marczak I will review this within this week.

kamil1marczak · 2021-06-18T03:58:39Z

Thanks @kamil1marczak I will review this within this week.

@alicertel hey any progress?

alicertel

Thanks @kamil1marczak I will review this within this week.

@alicertel hey any progress?

Apologies for the late review. I've been reviewing your PR but it's not finished. I will send it anyway and continue reviewing this week.

@omerfarukabaci Could you please help me review this PR?

tests/models.py

alicertel · 2021-06-28T11:21:25Z

requirements.txt

@@ -1,2 +1,3 @@
 Django >= 2.2
 djangorestframework >= 3.9.2
+cryptography==3.2.1


How about we make cryptography an optional dependency? Please see https://github.com/Hipo/drf-extra-fields/blob/master/setup.py#L19

Pillow is an optional dependency and it's only required if Base64ImageField is used.

We can raise an error if the cryptography is not installed when the one of the crypto fields are initialized. How that sounds?

alicertel · 2021-06-28T11:40:31Z

drf_extra_fields/crypto_fields.py

+            except InvalidToken:
+                return None


Why this is failing silently? There is something wrong here and this code block is hiding it. I think we should raise the error. What do you think?

alicertel · 2021-06-28T11:41:54Z

drf_extra_fields/crypto_fields.py

+        value = super(CryptoCharField, self).to_internal_value(value)
+        if value:
+            return value.decode("utf-8")
+        self.fail("invalid")


It's possible value to be None https://github.com/Hipo/drf-extra-fields/pull/160/files#diff-d6824db8ef32e9fad7a3e9a7ba84738d5a9c233da178e29aea2479e13e40f8fbR92

It will fail in that case but it shouldn't

alicertel · 2021-06-28T11:44:35Z

README.md

+## CryptoBinaryField and CryptoCharField
+
+ A django-rest-framework fields for handling encryption through serialisation. Inputs are String object and internal
+python representation is Binary object for CryptoBinaryField and String object for CryptoCharField
+
+ It takes the optional parameter `salt` (Django SECRET_KEY imported from setting as default). If set it use custom
+  cryptographic salt
+ It takes the optional parameter `password` ("Non_nobis1solum?nati!sumus" as default). If set it use a custom password
+  in encryption. **It is highly recommended to use custom one!!**
+ It takes the optional parameter `ttl` (None as default). If set it manage the number of seconds old a message may be
+  for it to be valid. If the message is older than ttl seconds (from the time it was originally created) field will
+  return None and encrypted message will not be enabled for decryption.
+
+ **Example**
+
+```python
+from rest_framework import serializers
+from drf_extra_fields.crypto_fields import CryptoCharField
+
+
+class CryptoSerializer(serializers.Serializer):
+  crypto_char = CryptoCharField()
+
+```
+ **Example with parameters**
+ It takes custom salt and password. Once saved it will be aviable for decryption for 1000 seconds.
+
+```python
+from rest_framework import serializers
+from drf_extra_fields.crypto_fields import CryptoCharField
+
+
+class CryptoSerializer(serializers.Serializer):
+  crypto_char = CryptoCharField(salt="custom salt", password="custom password", ttl=1000)
+
+```
+


I am not sure why there are bunch of changes in the README file. Could you please revert them back and only have the documentation of the newly added fields.

I assume these changes caused by the IDE/Editor you are using.

Agreed, it might be because of auto formatting or something.

alicertel · 2021-06-28T11:57:00Z

drf_extra_fields/crypto_fields.py

+    return decrypted_message.decode("utf-8")
+
+
+class CryptoBinaryField(serializers.Field):


Could you please tell me your use case for this field? I am curious about why you want to store binary data in DB (judging by your previous PR I am assuming these data will go to db)?

omerfarukabaci

Thank you for the changes! 🚀 I have made some change requests.

omerfarukabaci · 2021-06-29T12:24:01Z

README.md

+## CryptoBinaryField and CryptoCharField
+
+ A django-rest-framework fields for handling encryption through serialisation. Inputs are String object and internal
+python representation is Binary object for CryptoBinaryField and String object for CryptoCharField
+
+ It takes the optional parameter `salt` (Django SECRET_KEY imported from setting as default). If set it use custom
+  cryptographic salt
+ It takes the optional parameter `password` ("Non_nobis1solum?nati!sumus" as default). If set it use a custom password
+  in encryption. **It is highly recommended to use custom one!!**
+ It takes the optional parameter `ttl` (None as default). If set it manage the number of seconds old a message may be
+  for it to be valid. If the message is older than ttl seconds (from the time it was originally created) field will
+  return None and encrypted message will not be enabled for decryption.
+
+ **Example**
+
+```python
+from rest_framework import serializers
+from drf_extra_fields.crypto_fields import CryptoCharField
+
+
+class CryptoSerializer(serializers.Serializer):
+  crypto_char = CryptoCharField()
+
+```
+ **Example with parameters**
+ It takes custom salt and password. Once saved it will be aviable for decryption for 1000 seconds.
+
+```python
+from rest_framework import serializers
+from drf_extra_fields.crypto_fields import CryptoCharField
+
+
+class CryptoSerializer(serializers.Serializer):
+  crypto_char = CryptoCharField(salt="custom salt", password="custom password", ttl=1000)
+
+```
+


Agreed, it might be because of auto formatting or something.

README.md

tests/test_crypto_fields.py

omerfarukabaci · 2021-08-16T10:36:06Z

tests/test_crypto_fields.py

+class CryptoSerializer(serializers.Serializer):
+    message = CryptoBinaryField(required=False)


Let's call this BaseCryptoSerializer and create other serializers in the test methods using this:

def test_serialization_with_salt(self): class CryptoSerializer(BaseCryptoSerializer): message = CryptoBinaryField(password=..., salt=...)

tests/test_crypto_fields.py

omerfarukabaci · 2021-08-16T10:41:38Z

tests/test_crypto_fields.py

+        encrypted_message = _encrypt(token, message)
+        model_data = SaveCrypto(message=encrypted_message, created=now)
+        serializer = TtlCryptoSerializerSerializer(model_data)
+        time.sleep(3)


We cannot use time.sleep in tests, we should mock time.time.

omerfarukabaci · 2021-08-16T10:49:29Z

tests/test_crypto_fields.py

+    created = serializers.DateTimeField()
+
+
+class PointSerializerTest(TestCase):


Let's make sure that every AssertionErrror and ValidationError is tested.

Co-authored-by: Ömer Faruk Abacı <[email protected]>

ADD crypto-fields

3ef2c06

Add: Test and passing password

dd56211

alicertel reviewed May 11, 2021

View reviewed changes

alicertel requested a review from omerfarukabaci May 11, 2021 12:44

omerfarukabaci suggested changes May 11, 2021

View reviewed changes

kamil1marczak added 2 commits May 13, 2021 07:40

FIXED: rewritten to drf fields

d09eae6

FIXED: flake8

e239900

kamil1marczak added 3 commits May 13, 2021 14:23

FIX: more meaningful kwargs

e4196cc

FIX: cryptography downgraded to version that supports python 3.5

a721f2d

FIX: minor stylistic changes

714ecae

kamil1marczak requested review from omerfarukabaci and alicertel May 13, 2021 19:21

alicertel reviewed Jun 29, 2021

View reviewed changes

change after CR

9592896

omerfarukabaci suggested changes Aug 16, 2021

View reviewed changes

kamil1marczak and others added 8 commits August 27, 2021 19:57

Update README.md

7a42cd7

Co-authored-by: Ömer Faruk Abacı <[email protected]>

Update README.md

f10e622

Co-authored-by: Ömer Faruk Abacı <[email protected]>

Update README.md

a9b1d0d

Co-authored-by: Ömer Faruk Abacı <[email protected]>

Update tests/test_crypto_fields.py

5537e70

Co-authored-by: Ömer Faruk Abacı <[email protected]>

Update tests/test_crypto_fields.py

1450b47

Co-authored-by: Ömer Faruk Abacı <[email protected]>

Update README.md

5e8fe73

Co-authored-by: Ömer Faruk Abacı <[email protected]>

Update drf_extra_fields/crypto_fields.py

15a687c

Co-authored-by: Ömer Faruk Abacı <[email protected]>

Update README.md

f6f1e41

Co-authored-by: Ömer Faruk Abacı <[email protected]>

kamil1marczak and others added 3 commits September 20, 2021 20:36

Update tox.ini

b73f534

Co-authored-by: Ömer Faruk Abacı <[email protected]>

Update drf_extra_fields/crypto_fields.py

e037ec5

Co-authored-by: Ömer Faruk Abacı <[email protected]>

Update drf_extra_fields/crypto_fields.py

33e23cf

Co-authored-by: Ömer Faruk Abacı <[email protected]>

-**Settings.**
-each CryptoField has 2 kwargs *salt_settings_env* and *password*.
-* *salt_settings_env* - name of variable stored in settings.py file, which will be used as cryptographic salt. default: salt_settings_env = 'SECRET_KEY' if settings not set or no SECRET_KEY in setting default: salt = "Salt123!!!"
-* *password* - password to be used in encryption process of given field (together with salt set globally) default = 'password'
+### Settings
+Each CryptoField has 2 kwargs: `salt_settings_env` and `password`.
+* `salt_settings_env`: Name of the variable stored in the settings.py file, which will be used as cryptographic salt.
+**Default**: `"SECRET_KEY"`
+**Note**: If `salt_settings_env` does not exist in `settings.py` then `salt` will be `Salt123!!!`
+* `password`: Password to be used in the encryption process of the given field (together with salt set globally).
+**Default**: `"password"`

	class PersonTest(TestCase):
	class CryptoFieldsTests(TestCase):

		return decrypted_message.decode("utf-8")


		class CryptoBinaryField(serializers.Field):

		class CryptoSerializer(serializers.Serializer):
		message = CryptoBinaryField(required=False)

		created = serializers.DateTimeField()


		class PointSerializerTest(TestCase):

ADD crypto-fields #160

Are you sure you want to change the base?

ADD crypto-fields #160

Conversation

kamil1marczak commented Apr 24, 2021

codecov-commenter commented Apr 24, 2021 • edited Loading

Codecov Report

yigitguler commented May 10, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

omerfarukabaci left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alicertel commented May 11, 2021

kamil1marczak commented May 12, 2021 via email

kamil1marczak commented May 13, 2021

alicertel commented May 18, 2021

kamil1marczak commented Jun 18, 2021

alicertel left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

omerfarukabaci left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov-commenter commented Apr 24, 2021 •

edited

Loading

omerfarukabaci left a comment •

edited

Loading